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 385F243CA8 for ; Wed, 13 Mar 2024 21:27:09 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2B46D42E27; Wed, 13 Mar 2024 21:27:09 +0100 (CET) Received: from mail-lj1-f174.google.com (mail-lj1-f174.google.com [209.85.208.174]) by mails.dpdk.org (Postfix) with ESMTP id EA6D840151; Wed, 13 Mar 2024 21:27:05 +0100 (CET) Received: by mail-lj1-f174.google.com with SMTP id 38308e7fff4ca-2d4725c7c33so2883031fa.1; Wed, 13 Mar 2024 13:27:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1710361625; x=1710966425; 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=DSOxe4ncELMjiyzZG+xEcCIIBhW2z+G28AHdolYo4bI=; b=Qc1tkyfR5adXtjZ/ca/bCM//40NRfRuVDvmkP2Ge5uAwI4dtn77N5+HJE+gffANna5 rMpiU1uqlsZag20p5lgJdkApSVmTghK1rf4PLSCr6fEniOzrHYd4ohX5zym13nvfrgLR 4sVrdGYURr7Qpu9qGZf02WxMswwc4rZdEX/b5BTHz8z5+xUJ+USjqNiqLk3+s552reB5 dgWTpptf9AeB3WArkjDdU4SBSkztn2EzmPqmym/tfGq6rC9stlOkHXUd4AqK3euAbk4w HxxEQ75SFe2K9r3hFkwM2sHKYkJic+Rjw4hEUQFgVdTtlIP6tl1YDYm/AlnA7oSe4IsY porw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1710361625; x=1710966425; 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=DSOxe4ncELMjiyzZG+xEcCIIBhW2z+G28AHdolYo4bI=; b=kN6IfHR53GAh4crZ0oOSVAxw4FDE0J9rMWVZAvF5vgOc8kEQ5E0IybpPW777csL8WX 4HGUvktMo9HWXbVIHjN03+SFBFeYk83S2954avTnhR26lAHlpk5zmMdQ6qo2fQ3yE7Kj VuBb7Ks4BoQXv1nn2hRPRKchESzal6MnDj/9qQDRyWDevtq6nvLST5ILbrFYOr5tetJm ByDBRy7WTvAo89l/8YMoD7LD4hLGEv11ZH3mRoV0k3op3Krz3wcDmqtXOKogR7lfkqBH bpNKJHzLqOk/ys4kz4e/6SHID1zmsgYYtmeMxemkSXbHZIPUUKMEae/0wYiIhZnGfDIf AcgA== X-Forwarded-Encrypted: i=1; AJvYcCXzkszLvl5gLHei8SzVER8G200raPEbOxTtXQryFJkM2Xe54h+WxWokaYXmfsQ3MBU4iLrPvq+LwG7XFWjHWXeqV7dfGArWMkVLQi7swg== X-Gm-Message-State: AOJu0Yzs51cvaqx0h9MV+LPoJt/ElbLidCKHQHQeCgTkchrNl1AXHOeX OCl+6I7zBow3PAX5dYPrvACZILTjSb/t57D91IXBzoTWOXv8muln42ITVCz8i+cjJhW+FL+qmC4 K8Wb4bCAhSnK4jKDigXjtABGVEKI= X-Google-Smtp-Source: AGHT+IFYP8d9x/OC8v6GKKgLJs93V3ecJXvkHMc9XxuQbSemCwBYCrLEgAOjs32UCJALxMpIL6J3WqbfV2urmARZaDI= X-Received: by 2002:a2e:9048:0:b0:2d4:2bee:e550 with SMTP id n8-20020a2e9048000000b002d42beee550mr9561365ljg.28.1710361624953; Wed, 13 Mar 2024 13:27:04 -0700 (PDT) MIME-Version: 1.0 References: <20240202051335.776290-1-ashish.sadanandan@gmail.com> <5751086.DvuYhMxLoT@thomas> <20240205173652.GA2935@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <98CBD80474FA8B44BF855DF32C47DC35E9F1EA@smartserver.smartshare.dk> <3fda997a-8e3d-447d-9b6d-01d48defb670@amd.com> <98CBD80474FA8B44BF855DF32C47DC35E9F208@smartserver.smartshare.dk> In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35E9F208@smartserver.smartshare.dk> From: Ashish Sadanandan Date: Wed, 13 Mar 2024 14:26:38 -0600 Message-ID: Subject: Re: [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h To: =?UTF-8?Q?Morten_Br=C3=B8rup?= Cc: Ferruh Yigit , Tyler Retzlaff , Bruce Richardson , Thomas Monjalon , 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="00000000000017cccd0613909976" X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org --00000000000017cccd0613909976 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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) they > > 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 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 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 having > > >> 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 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 somewhere? > > Good point! > > It could be added to =C2=A7 1.4.2. Header File Guards in the Coding Style > chapter of the Contributor's Guidelines. > > BTW, that paragraph (and its example) should be updated to reflect 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 someone need to sign off on this change before it can be merged? Thanks, Ashish --00000000000017cccd0613909976 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Mon, Feb 12, 2024 at 9:02=E2=80=AF= AM Morten Br=C3=B8rup <mb@sm= artsharesystems.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 wr= ote:
> >>> 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" d= irective which causes name
> >>>>> mangling of functions by C++ compilers, leading t= o linker errors
> >>>>> complaining of undefined references to these func= tions.
> >>>>>
> >>>>> Fixes: 86c743cf9140 ("eal: define generic ve= ctor types")
> >>>>> Cc: nelio.laranjeiro@6wind.com
> >>>>> Cc: stable@dpdk.org
> >>>>>
> >>>>> Signed-off-by: Ashish Sadanandan <ashish.sadanandan@gmai= l.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 i= s not directly included
> >>>> 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 e= xtern "C"
> >> block
> >>>> in lib/eal/*/include/rte_vect.h (instead of being bef= ore the
> >> block).
> >>>>
> >>>> I would like we use the same approach for all files.<= br> > >>>> 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 fi= le that it is
> >> correct.
> >>> Taking the other option, to check any indirect include fi= le you
> need
> >> to go
> >>> finding what other files include it and check there that = a) they
> 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 ever= y 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 t= he 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 &qu= ot;C"' bit]
> >>
> >> so first, i agree with what you say here. but one downside i&= #39;ve seen
> >> is that non-public symbols may end up as extern "C"= .
> >>
> >> i've also been unsatisfied with the inconsistency of eith= er 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 with always i= ncluding inside
> the
> >> block is we can't reliably provide a offer a C++-only hea= der without
> >> doing extern "C++".
> >
> > I would say that the first of the two above patterns is not only<= br> > annoying, it is incorrect.
> > A DPDK header file should not change the meaning of any other hea= der
> 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 Style c= hapter of the Contributor's Guidelines.

BTW, that paragraph (and its example) should be updated to reflect that alp= habetical order is preferred.

Was the i= ntent 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?

Thanks,
Ashish
--00000000000017cccd0613909976--