From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <stable-bounces@dpdk.org>
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
by inbox.dpdk.org (Postfix) with ESMTP id 0276443CAD
for <public@inbox.dpdk.org>; Wed, 13 Mar 2024 23:11:58 +0100 (CET)
Received: from mails.dpdk.org (localhost [127.0.0.1])
by mails.dpdk.org (Postfix) with ESMTP id E85F042E33;
Wed, 13 Mar 2024 23:11:57 +0100 (CET)
Received: from mail-wr1-f51.google.com (mail-wr1-f51.google.com
[209.85.221.51])
by mails.dpdk.org (Postfix) with ESMTP id 0DEF940151;
Wed, 13 Mar 2024 23:11:55 +0100 (CET)
Received: by mail-wr1-f51.google.com with SMTP id
ffacd0b85a97d-33e94c12cfaso217379f8f.3;
Wed, 13 Mar 2024 15:11:55 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=gmail.com; s=20230601; t=1710367914; x=1710972714; darn=dpdk.org;
h=cc:to:subject:message-id:date:from:in-reply-to:references
:mime-version:from:to:cc:subject:date:message-id:reply-to;
bh=pmz5IHoYJqWaGIH3DzShIYt6RJaRjMCRwKxN2sAZ9Ik=;
b=QldoWkKC2K8IPTAarRnozSHrV2YS6vgO4PjgdXedkgDgAeDsMLMMgDHFJB0siysxwp
bfwQedMIoXpwALUti+EE7wnWrxdh3Yd2cM79xEEbb9K0BJy9eqGiL/a9hbD2QdfvWoI3
Atq2nR8bVnms1Ly+cQWhgoJn5S8gnjD47ZpBjDb/mG34UVRQvL29LgJeYyx+akqHocot
crZ8qPoDTh8dcioWh34wMDuKyh8dL1ZyBGEX7QKMdSwQbogI/q6WkTW+W+C3Nb1R3+4Q
1GT7JoCw2PzzvE17saZ9we9Vn9dNtgRhsnlifYnpHK+NIqA0dbqJZ4sO9xUdYZYcGMk2
CmGA==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20230601; t=1710367914; x=1710972714;
h=cc:to:subject:message-id:date:from:in-reply-to:references
:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id
:reply-to;
bh=pmz5IHoYJqWaGIH3DzShIYt6RJaRjMCRwKxN2sAZ9Ik=;
b=lDIPfkoWOh9ahARHkDBYcbzK7fpKFHkC17+FSEg/bZ/u/IA6b4enzi24k+ZzDrcZ0q
gCXOxX29uglzRersAL4zQqahlBDmvjQYMokKuz56Dw2XBf+9ovSTR2LUOwRzS8tYcP2b
qwgPG2SuHSzppVvor1mNy22V6hz6xHCYjeBH2t4y/o6erpasaSJOVGAYUFcM1I5UkSYh
olTUhfcRndT2rY3r4Ed+tx4FeL4kyVRZQwE3Ozfg312d2Du2Dqg4a4trOaocHpUf7Ybj
qIomr//XgbrMUF6vWsNzf17C8omeY+C670FLXYtfwJ+NXwGDyk+W+Bg04LmmIt/8hmDh
m2pQ==
X-Forwarded-Encrypted: i=1;
AJvYcCVUUBsGM66G5p+/l+4ljAYiZV6r5trANCTw/EBi6kByS4RkY3qXmarl+azwP7FqPiHH+VGIjbcLuH2l7D7k7iiB9Vle6LbvwsC9+2NoFQ==
X-Gm-Message-State: AOJu0Yxcaq0ClDj4+8OS9ZZlQ7wzSh/XjeqAK0EiaYcte8VNw99uG2Nh
STkAmF3GCfKZgBu+EByjLnTF4LUCAVHrl6GjsbEloK4Vw7nBaspR123U5NNtpCpX0a91AlkKi/X
oXlbscEC2pyCghK8pEXUqZ4hzA5g=
X-Google-Smtp-Source: AGHT+IHvDze5iNs44r3Xv4z/KZ0Qf3UJW45FWikPSpl3BqZsYKQnmxN9eCAPwnYmISPaE1+po9Bz6WeUaSGGYG3Rzp4=
X-Received: by 2002:a05:6000:102:b0:33e:c0c5:1795 with SMTP id
o2-20020a056000010200b0033ec0c51795mr845227wrx.54.1710367914417; Wed, 13 Mar
2024 15:11:54 -0700 (PDT)
MIME-Version: 1.0
References: <20240202051335.776290-1-ashish.sadanandan@gmail.com>
<98CBD80474FA8B44BF855DF32C47DC35E9F208@smartserver.smartshare.dk>
<CAN44U20V75doRWd1yQTzwjau+560ERi3wwK1gLTN932ymWan2g@mail.gmail.com>
<6606679.G0QQBjFxQf@thomas>
In-Reply-To: <6606679.G0QQBjFxQf@thomas>
From: Ashish Sadanandan <ashish.sadanandan@gmail.com>
Date: Wed, 13 Mar 2024 16:11:27 -0600
Message-ID: <CAN44U224jksFvm0JrfenwBq4PhudY3QeS7wWHAQbSKw-b5s0Xg@mail.gmail.com>
Subject: Re: [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h
To: Thomas Monjalon <thomas@monjalon.net>
Cc: =?UTF-8?Q?Morten_Br=C3=B8rup?= <mb@smartsharesystems.com>,
Ferruh Yigit <ferruh.yigit@amd.com>,
Tyler Retzlaff <roretzla@linux.microsoft.com>,
Bruce Richardson <bruce.richardson@intel.com>, dev@dpdk.org,
nelio.laranjeiro@6wind.com,
stable@dpdk.org, honnappa.nagarahalli@arm.com, konstantin.v.ananyev@yandex.ru,
david.marchand@redhat.com, ruifeng.wang@arm.com
Content-Type: multipart/alternative; boundary="000000000000f96d6a0613920feb"
X-BeenThere: stable@dpdk.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: patches for DPDK stable branches <stable.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/stable>,
<mailto:stable-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/stable/>
List-Post: <mailto:stable@dpdk.org>
List-Help: <mailto:stable-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/stable>,
<mailto:stable-request@dpdk.org?subject=subscribe>
Errors-To: stable-bounces@dpdk.org
--000000000000f96d6a0613920feb
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
On Wed, Mar 13, 2024 at 2:45=E2=80=AFPM Thomas Monjalon <thomas@monjalon.ne=
t> wrote:
> 13/03/2024 21:26, Ashish Sadanandan:
> > On Mon, Feb 12, 2024 at 9:02=E2=80=AFAM Morten Br=C3=B8rup <mb@smartsha=
resystems.com>
> > wrote:
> >
> > > > From: Ferruh Yigit [mailto:ferruh.yigit@amd.com]
> > > > Sent: Monday, 12 February 2024 16.43
> > > >
> > > > On 2/5/2024 9:07 PM, Morten Br=C3=B8rup wrote:
> > > > >> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > > >> Sent: Monday, 5 February 2024 18.37
> > > > >>
> > > > >> On Fri, Feb 02, 2024 at 09:40:59AM +0000, Bruce Richardson wrote=
:
> > > > >>> On Fri, Feb 02, 2024 at 10:18:23AM +0100, Thomas Monjalon wrote=
:
> > > > >>>> 02/02/2024 06:13, Ashish Sadanandan:
> > > > >>>>> The header was missing the extern "C" directive which causes
> name
> > > > >>>>> mangling of functions by C++ compilers, leading to linker
> errors
> > > > >>>>> complaining of undefined references to these functions.
> > > > >>>>>
> > > > >>>>> Fixes: 86c743cf9140 ("eal: define generic vector types")
> > > > >>>>> Cc: nelio.laranjeiro@6wind.com
> > > > >>>>> Cc: stable@dpdk.org
> > > > >>>>>
> > > > >>>>> Signed-off-by: Ashish Sadanandan <ashish.sadanandan@gmail.com=
>
> > > > >>>>
> > > > >>>> Thank you for improving C++ compatibility.
> > > > >>>>
> > > > >>>> I'm not sure what is best to fix it.
> > > > >>>> You are adding extern "C" in a file which is not directly
> included
> > > > >>>> by the user app. The same was done for rte_rwlock.h.
> > > > >>>> The other way is to make sure this include is in an extern "C"
> > > > >> block
> > > > >>>> in lib/eal/*/include/rte_vect.h (instead of being before the
> > > > >> block).
> > > > >>>>
> > > > >>>> I would like we use the same approach for all files.
> > > > >>>> Opinions?
> > > > >>>>
> > > > >>> I think just having the extern "C" guard in all files is the
> safest
> > > > >> choice,
> > > > >>> because it's immediately obvious in each and every file that it
> is
> > > > >> correct.
> > > > >>> Taking the other option, to check any indirect include file you
> > > > need
> > > > >> to go
> > > > >>> finding what other files include it and check there that a) the=
y
> > > > have
> > > > >>> include guards and b) the include for the indirect header is
> > > > >> contained
> > > > >>> within it.
> > > > >>>
> > > > >>> Adopting the policy of putting the guard in each and every head=
er
> > > > is
> > > > >> also a
> > > > >>> lot easier to do basic automated sanity checks on. If the file
> ends
> > > > >> in .h,
> > > > >>> we just use grep to quickly verify it's not missing the guards.
> > > > >> [Naturally,
> > > > >>> we can do more complete checks than that if we want, but 99%
> > > > percent
> > > > >> of
> > > > >>> misses can be picked up by a grep for the 'extern "C"' bit]
> > > > >>
> > > > >> so first, i agree with what you say here. but one downside i've
> seen
> > > > >> is that non-public symbols may end up as extern "C".
> > > > >>
> > > > >> i've also been unsatisfied with the inconsistency of either havi=
ng
> > > > >> includes in or outside of the guards.
> > > > >>
> > > > >> a lot of dpdk headers follow this pattern.
> > > > >>
> > > > >> // foo.h
> > > > >> #ifdef __cplusplus
> > > > >> extern "C" {
> > > > >> #endif
> > > > >>
> > > > >> #include <stdio.h>
> > > > >>
> > > > >> ...
> > > > >>
> > > > >> but some dpdk headers follow this pattern.
> > > > >>
> > > > >> // foo.h
> > > > >> #include <stdio.h>
> > > > >>
> > > > >> #ifdef __cplusplus
> > > > >> extern "C"
> > > > >> #endif
> > > > >>
> > > > >> ...
> > > > >>
> > > > >> standard C headers include the guards so don't need to be inside
> the
> > > > >> extern "C" block. one minor annoyance with always including insi=
de
> > > > the
> > > > >> block is we can't reliably provide a offer a C++-only header
> without
> > > > >> doing extern "C++".
> > > > >
> > > > > I would say that the first of the two above patterns is not only
> > > > annoying, it is incorrect.
> > > > > A DPDK header file should not change the meaning of any other
> header
> > > > files it includes.
> > > > > And although the incorrectness currently only screws up any C++ i=
n
> > > > those header files, I still consider it a bug.
> > > > >
> > > >
> > > > Should we document the proper extern "C" usage somewhere?
> > >
> > > Good point!
> > >
> > > It could be added to =C2=A7 1.4.2. Header File Guards in the Coding S=
tyle
> > > chapter of the Contributor's Guidelines.
> > >
> > > BTW, that paragraph (and its example) should be updated to reflect th=
at
> > > alphabetical order is preferred.
> > >
> >
> > Was the intent of this comment that I should include this update in my
> > patch? I'm happy to do it, but IMO the guideline update should be a
> > separate commit.
> >
> > It's been a month since the last activity on this thread, does someone
> need
> > to sign off on this change before it can be merged?
>
> Instead of doing one specific change, it would be better to change all
> files for consistency.
> So the guideline change can be in the same commit applying the new
> guideline.
>
>
> Fair enough, I'll take a crack at it tonight.
- Ashish
--000000000000f96d6a0613920feb
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
<div dir=3D"ltr"><div dir=3D"ltr"><br></div><br><div class=3D"gmail_quote">=
<div dir=3D"ltr" class=3D"gmail_attr">On Wed, Mar 13, 2024 at 2:45=E2=80=AF=
PM Thomas Monjalon <<a href=3D"mailto:thomas@monjalon.net">thomas@monjal=
on.net</a>> wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"m=
argin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left=
:1ex">13/03/2024 21:26, Ashish Sadanandan:<br>
> On Mon, Feb 12, 2024 at 9:02=E2=80=AFAM Morten Br=C3=B8rup <<a href=
=3D"mailto:mb@smartsharesystems.com" target=3D"_blank">mb@smartsharesystems=
.com</a>><br>
> wrote:<br>
> <br>
> > > From: Ferruh Yigit [mailto:<a href=3D"mailto:ferruh.yigit@am=
d.com" target=3D"_blank">ferruh.yigit@amd.com</a>]<br>
> > > Sent: Monday, 12 February 2024 16.43<br>
> > ><br>
> > > On 2/5/2024 9:07 PM, Morten Br=C3=B8rup wrote:<br>
> > > >> From: Tyler Retzlaff [mailto:<a href=3D"mailto:rore=
tzla@linux.microsoft.com" target=3D"_blank">roretzla@linux.microsoft.com</a=
>]<br>
> > > >> Sent: Monday, 5 February 2024 18.37<br>
> > > >><br>
> > > >> On Fri, Feb 02, 2024 at 09:40:59AM +0000, Bruce Ric=
hardson wrote:<br>
> > > >>> On Fri, Feb 02, 2024 at 10:18:23AM +0100, Thoma=
s Monjalon wrote:<br>
> > > >>>> 02/02/2024 06:13, Ashish Sadanandan:<br>
> > > >>>>> The header was missing the extern "=
;C" directive which causes name<br>
> > > >>>>> mangling of functions by C++ compilers,=
leading to linker errors<br>
> > > >>>>> complaining of undefined references to =
these functions.<br>
> > > >>>>><br>
> > > >>>>> Fixes: 86c743cf9140 ("eal: define =
generic vector types")<br>
> > > >>>>> Cc: <a href=3D"mailto:nelio.laranjeiro@=
6wind.com" target=3D"_blank">nelio.laranjeiro@6wind.com</a><br>
> > > >>>>> Cc: <a href=3D"mailto:stable@dpdk.org" =
target=3D"_blank">stable@dpdk.org</a><br>
> > > >>>>><br>
> > > >>>>> Signed-off-by: Ashish Sadanandan <<a=
href=3D"mailto:ashish.sadanandan@gmail.com" target=3D"_blank">ashish.sadan=
andan@gmail.com</a>><br>
> > > >>>><br>
> > > >>>> Thank you for improving C++ compatibility.<=
br>
> > > >>>><br>
> > > >>>> I'm not sure what is best to fix it.<br=
>
> > > >>>> You are adding extern "C" in a fi=
le which is not directly included<br>
> > > >>>> by the user app. The same was done for rte_=
rwlock.h.<br>
> > > >>>> The other way is to make sure this include =
is in an extern "C"<br>
> > > >> block<br>
> > > >>>> in lib/eal/*/include/rte_vect.h (instead of=
being before the<br>
> > > >> block).<br>
> > > >>>><br>
> > > >>>> I would like we use the same approach for a=
ll files.<br>
> > > >>>> Opinions?<br>
> > > >>>><br>
> > > >>> I think just having the extern "C" gu=
ard in all files is the safest<br>
> > > >> choice,<br>
> > > >>> because it's immediately obvious in each an=
d every file that it is<br>
> > > >> correct.<br>
> > > >>> Taking the other option, to check any indirect =
include file you<br>
> > > need<br>
> > > >> to go<br>
> > > >>> finding what other files include it and check t=
here that a) they<br>
> > > have<br>
> > > >>> include guards and b) the include for the indir=
ect header is<br>
> > > >> contained<br>
> > > >>> within it.<br>
> > > >>><br>
> > > >>> Adopting the policy of putting the guard in eac=
h and every header<br>
> > > is<br>
> > > >> also a<br>
> > > >>> lot easier to do basic automated sanity checks =
on. If the file ends<br>
> > > >> in .h,<br>
> > > >>> we just use grep to quickly verify it's not=
missing the guards.<br>
> > > >> [Naturally,<br>
> > > >>> we can do more complete checks than that if we =
want, but 99%<br>
> > > percent<br>
> > > >> of<br>
> > > >>> misses can be picked up by a grep for the '=
extern "C"' bit]<br>
> > > >><br>
> > > >> so first, i agree with what you say here. but one d=
ownside i've seen<br>
> > > >> is that non-public symbols may end up as extern &qu=
ot;C".<br>
> > > >><br>
> > > >> i've also been unsatisfied with the inconsisten=
cy of either having<br>
> > > >> includes in or outside of the guards.<br>
> > > >><br>
> > > >> a lot of dpdk headers follow this pattern.<br>
> > > >><br>
> > > >> // foo.h<br>
> > > >> #ifdef __cplusplus<br>
> > > >> extern "C" {<br>
> > > >> #endif<br>
> > > >><br>
> > > >> #include <stdio.h><br>
> > > >><br>
> > > >> ...<br>
> > > >><br>
> > > >> but some dpdk headers follow this pattern.<br>
> > > >><br>
> > > >> // foo.h<br>
> > > >> #include <stdio.h><br>
> > > >><br>
> > > >> #ifdef __cplusplus<br>
> > > >> extern "C"<br>
> > > >> #endif<br>
> > > >><br>
> > > >> ...<br>
> > > >><br>
> > > >> standard C headers include the guards so don't =
need to be inside the<br>
> > > >> extern "C" block. one minor annoyance wit=
h always including inside<br>
> > > the<br>
> > > >> block is we can't reliably provide a offer a C+=
+-only header without<br>
> > > >> doing extern "C++".<br>
> > > ><br>
> > > > I would say that the first of the two above patterns is=
not only<br>
> > > annoying, it is incorrect.<br>
> > > > A DPDK header file should not change the meaning of any=
other header<br>
> > > files it includes.<br>
> > > > And although the incorrectness currently only screws up=
any C++ in<br>
> > > those header files, I still consider it a bug.<br>
> > > ><br>
> > ><br>
> > > Should we document the proper extern "C" usage som=
ewhere?<br>
> ><br>
> > Good point!<br>
> ><br>
> > It could be added to =C2=A7 1.4.2. Header File Guards in the Codi=
ng Style<br>
> > chapter of the Contributor's Guidelines.<br>
> ><br>
> > BTW, that paragraph (and its example) should be updated to reflec=
t that<br>
> > alphabetical order is preferred.<br>
> ><br>
> <br>
> Was the intent of this comment that I should include this update in my=
<br>
> patch? I'm happy to do it, but IMO the guideline update should be =
a<br>
> separate commit.<br>
> <br>
> It's been a month since the last activity on this thread, does som=
eone need<br>
> to sign off on this change before it can be merged?<br>
<br>
Instead of doing one specific change, it would be better to change all file=
s for consistency.<br>
So the guideline change can be in the same commit applying the new guidelin=
e.<br>
<br>
<br></blockquote><div>=C2=A0Fair enough, I'll take a crack at it tonigh=
t.</div><div><br></div><div>- Ashish<br></div></div></div>
--000000000000f96d6a0613920feb--