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

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 2/5/2024 9:07 PM, Morten Brørup 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 <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.
> 

Should we document the proper extern "C" usage somewhere?