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

[Tyler Retzlaff <roretzla@linux.microsoft.com>]

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++".

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

> 
> /Bruce