From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id C020343CAC; Wed, 13 Mar 2024 23:11:56 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 921D24025D; Wed, 13 Mar 2024 23:11:56 +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> <6606679.G0QQBjFxQf@thomas> In-Reply-To: <6606679.G0QQBjFxQf@thomas> From: Ashish Sadanandan Date: Wed, 13 Mar 2024 16:11:27 -0600 Message-ID: Subject: Re: [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h To: Thomas Monjalon Cc: =?UTF-8?Q?Morten_Br=C3=B8rup?= , Ferruh Yigit , Tyler Retzlaff , Bruce Richardson , 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: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-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 wrote: > 13/03/2024 21:26, Ashish Sadanandan: > > On Mon, Feb 12, 2024 at 9:02=E2=80=AFAM Morten Br=C3=B8rup > > 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 > > > > >>>> > > > > >>>> 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 > > > > >> > > > > >> ... > > > > >> > > > > >> but some dpdk headers follow this pattern. > > > > >> > > > > >> // foo.h > > > > >> #include > > > > >> > > > > >> #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


=
On Wed, Mar 13, 2024 at 2:45=E2=80=AF= PM Thomas Monjalon <thomas@monjal= on.net> wrote:
13/03/2024 21:26, Ashish Sadanandan:
> On Mon, Feb 12, 2024 at 9:02=E2=80=AFAM Morten Br=C3=B8rup <mb@smartsharesystems= .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 Ric= hardson wrote:
> > > >>> On Fri, Feb 02, 2024 at 10:18:23AM +0100, Thoma= s 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.sadan= andan@gmail.com>
> > > >>>>
> > > >>>> Thank you for improving C++ compatibility.<= br> > > > >>>>
> > > >>>> I'm not sure what is best to fix it. > > > >>>> You are adding extern "C" in a fi= le 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 a= ll files.
> > > >>>> Opinions?
> > > >>>>
> > > >>> I think just having the extern "C" gu= ard in all files is the safest
> > > >> choice,
> > > >>> because it's immediately obvious in each an= d 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 t= here that a) they
> > > have
> > > >>> include guards and b) the include for the indir= ect header is
> > > >> contained
> > > >>> within it.
> > > >>>
> > > >>> Adopting the policy of putting the guard in eac= h and every header
> > > 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 d= ownside i've seen
> > > >> is that non-public symbols may end up as extern &qu= ot;C".
> > > >>
> > > >> i've also been unsatisfied with the inconsisten= cy of either having
> > > >> 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 wit= h always including inside
> > > 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++ in
> > > those header files, I still consider it a bug.
> > > >
> > >
> > > Should we document the proper extern "C" usage som= ewhere?
> >
> > Good point!
> >
> > It could be added to =C2=A7 1.4.2. Header File Guards in the Codi= ng Style
> > chapter of the Contributor's Guidelines.
> >
> > BTW, that paragraph (and its example) should be updated to reflec= t that
> > 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 som= eone 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 file= s for consistency.
So the guideline change can be in the same commit applying the new guidelin= e.


=C2=A0Fair enough, I'll take a crack at it tonigh= t.

- Ashish
--000000000000f96d6a0613920feb--