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 CA88943CA8 for ; Wed, 13 Mar 2024 21:45:44 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C506642E27; Wed, 13 Mar 2024 21:45:44 +0100 (CET) Received: from fout6-smtp.messagingengine.com (fout6-smtp.messagingengine.com [103.168.172.149]) by mails.dpdk.org (Postfix) with ESMTP id 3A57D40151; Wed, 13 Mar 2024 21:45:42 +0100 (CET) Received: from compute7.internal (compute7.nyi.internal [10.202.2.48]) by mailfout.nyi.internal (Postfix) with ESMTP id 556231380068; Wed, 13 Mar 2024 16:45:41 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute7.internal (MEProxy); Wed, 13 Mar 2024 16:45:41 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:cc:content-transfer-encoding:content-type:content-type:date :date:from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:subject:subject:to:to; s=fm1; t=1710362741; x=1710449141; bh=etPt3GuIejpHxkRJaMzc4dIZbmlQPTjLBw2sMIL8SFE=; b= FCtIGoieERPG1/YBktl9vsc5LYNLqnr+XSFRzSWMRfPyK/f/gGbFaSKxzS1yr+64 MyhXIs/6LYopIWta55dlMg8Onl8bJi+TTxYJxL+RGrGQORK4bMpzDA8YLx7Zz6jf f7Jt4DfIa2jmKhCcKoJTP3qNRIU3hFSGeDL0LDTMQjkEmO7NNWH8qvLkGKgOLRiw ujk0HQm4yd4MU+N0w+TPXeILeLo3hVpig2usBvJwI9f/LGC/7UlVaNpIFu6X4g9A 5Ys3QpdICIVNsjXbTjFTUqyh5EZmdvzo4tAD66Q48Dbchagl503boKkqdG9QamPF aDrTAnCYqI+Cr7h0t1XIKg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:content-type:date:date:feedback-id:feedback-id :from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:subject:subject:to:to:x-me-proxy:x-me-proxy :x-me-sender:x-me-sender:x-sasl-enc; s=fm1; t=1710362741; x= 1710449141; bh=etPt3GuIejpHxkRJaMzc4dIZbmlQPTjLBw2sMIL8SFE=; b=S vwP1TA662gRQuDTcx0ddEd5BW+WeTdzyFC1DEOXxw5iokGrEfMkn5LymSCDSyu/j GN1tqp34UnydqKIS1Q65bABmGm1F33FRUtTZnCv63Sg1N4XESwIWI3rUgUPHWb2Q yXYmtnZUH9+qNrlv5CEo9IB5OLJIEsq5ikzuwTz6jKvAwne6+4RYHfRYv9JEW5Xe Cw7x654n1Ir0nn9na6jXOglaxl00BYrqvOQIeiX8u1qhEiZEB62hxSZ8u6icZFRo Rjd6No7cHxIP1Ow1xnNORTaXg573R5XLquEyNCIypBfVYMF0bYvGGqEdlXSk+iJO bE1I8pBVtTGYP1pMMEKfA== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvledrjeehgddugedtucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvvefufffkjghfggfgtgesthhqredttddtjeenucfhrhhomhepvfhhohhm rghsucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenuc ggtffrrghtthgvrhhnpeegtddtleejjeegffekkeektdejvedtheevtdekiedvueeuvdei uddvleevjeeujeenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfh hrohhmpehthhhomhgrshesmhhonhhjrghlohhnrdhnvght X-ME-Proxy: Feedback-ID: i47234305:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Wed, 13 Mar 2024 16:45:39 -0400 (EDT) From: Thomas Monjalon To: Morten =?ISO-8859-1?Q?Br=F8rup?= , Ashish Sadanandan Cc: 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 Subject: Re: [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h Date: Wed, 13 Mar 2024 21:45:37 +0100 Message-ID: <6606679.G0QQBjFxQf@thomas> In-Reply-To: References: <20240202051335.776290-1-ashish.sadanandan@gmail.com> <98CBD80474FA8B44BF855DF32C47DC35E9F208@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="utf-8" 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 13/03/2024 21:26, Ashish Sadanandan: > On Mon, Feb 12, 2024 at 9:02=E2=80=AFAM Morten Br=C3=B8rup > wrote: >=20 > > > 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 na= me > > > >>>>> 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 includ= ed > > > >>>> 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 safe= st > > > >> 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 en= ds > > > >> 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 se= en > > > >> 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 t= he > > > >> extern "C" block. one minor annoyance with always including inside > > > the > > > >> block is we can't reliably provide a offer a C++-only header witho= ut > > > >> 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 Sty= le > > chapter of the Contributor's Guidelines. > > > > BTW, that paragraph (and its example) should be updated to reflect that > > alphabetical order is preferred. > > >=20 > 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. >=20 > It's been a month since the last activity on this thread, does someone ne= ed > 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.