<div dir="ltr"><div dir="ltr"> </div> <div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Mar 13, 2024 at 2:45 PM Thomas Monjalon <<a href="mailto:thomas@monjalon.net">thomas@monjalon.net</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">13/03/2024 21:26, Ashish Sadanandan: > On Mon, Feb 12, 2024 at 9:02 AM Morten Brørup <<a href="mailto:mb@smartsharesystems.com" target="_blank">mb@smartsharesystems.com</a>> > wrote: > > > > From: Ferruh Yigit [mailto:<a href="mailto:ferruh.yigit@amd.com" target="_blank">ferruh.yigit@amd.com</a>] > > > Sent: Monday, 12 February 2024 16.43 > > > > > > On 2/5/2024 9:07 PM, Morten Brørup wrote: > > > >> From: Tyler Retzlaff [mailto:<a href="mailto:roretzla@linux.microsoft.com" target="_blank">roretzla@linux.microsoft.com</a>] > > > >> 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: <a href="mailto:nelio.laranjeiro@6wind.com" target="_blank">nelio.laranjeiro@6wind.com</a> > > > >>>>> Cc: <a href="mailto:stable@dpdk.org" target="_blank">stable@dpdk.org</a> > > > >>>>> > > > >>>>> Signed-off-by: Ashish Sadanandan <<a href="mailto:ashish.sadanandan@gmail.com" target="_blank">ashish.sadanandan@gmail.com</a>> > > > >>>> > > > >>>> 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 <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 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 § 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? 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. </blockquote><div> Fair enough, I'll take a crack at it tonight.</div><div> </div><div>- Ashish </div></div></div>