RE: [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h

["Morten Brørup" <mb@smartsharesystems.com>]

> 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) 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.

> 
> please bear in mind i do not mean to suggest implementing any dpdk in
> C++ with this comment, merely that there are advantages to occasionally
> offering C++-only header content to applications should we wever want
> to.
> 
> in some cases for harmony between C and C++ it may be easier to
> interoperate by supplying some basic C++-only headers, this may become
> more important as it there appears to be increasing divergance between
> the C and C++ standards and interoperability.
> 
> for full disclosure i do anticipate having to provide some small bits
> of
> header only C++ for msvc which is why this is top of my mind.
> 
> finally, i'll also note that we could again be explicit in our intent
> to
> declare what is extern "C" / exported by instead marking the declared
> names
> (functions and variables) themselves in a more precise manner.
> 
> i.e.
> __rte_<lib>_export // extern "C" or __declspec(dllexport) extern "C"
> void some_public_symbol(void);
> 
> you'll recall we had a related discussion about symbol visibility here
> which is somewhat a similar problem to being solved to symbol naming.
> if
> we were defaulting visibility to hidden and using a single mechanism to
> guarantee extern "C" linkage and public visibility exposure we could
> catch all "missed" symbols for C++ without having to build as C++ and
> reference the symbols.
> 
> https://mails.dpdk.org/archives/dev/2024-January/285109.html
> 
> i still intend to put forward an RFC for the discussion resulting from
> that thread (just haven't had time yet).

With that RFC, please also mention if function pointers need any special/additional considerations. No need to think about it yet. :-)

> 
> >
> > /Bruce