[PATCH v6 1/6] dpdk: do not force C linkage on include file dependencies

[Mattias Rönnblom]

On 2024-09-18 13:15, David Marchand wrote:
> On Tue, Sep 17, 2024 at 11:30 AM Mattias Rönnblom <hofors at lysator.liu.se> wrote:
>>
>> On 2024-09-16 14:05, David Marchand wrote:
>>> Hello,
>>>
>>> On Tue, Sep 10, 2024 at 10:41 AM Mattias Rönnblom
>>> <mattias.ronnblom at ericsson.com> wrote:
>>>> diff --git a/lib/acl/rte_acl_osdep.h b/lib/acl/rte_acl_osdep.h
>>>> index 3c1dc402ca..e4c7d07c69 100644
>>>> --- a/lib/acl/rte_acl_osdep.h
>>>> +++ b/lib/acl/rte_acl_osdep.h
>>>> @@ -5,10 +5,6 @@
>>>>    #ifndef _RTE_ACL_OSDEP_H_
>>>>    #define _RTE_ACL_OSDEP_H_
>>>>
>>>> -#ifdef __cplusplus
>>>> -extern "C" {
>>>> -#endif
>>>> -
>>>>    /**
>>>>     * @file
>>>>     *
>>>> @@ -49,6 +45,10 @@ extern "C" {
>>>>    #include <rte_cpuflags.h>
>>>>    #include <rte_debug.h>
>>>>
>>>> +#ifdef __cplusplus
>>>> +extern "C" {
>>>> +#endif
>>>> +
>>>>    #ifdef __cplusplus
>>>>    }
>>>>    #endif
>>>
>>> This part is a NOOP, so we can just drop it.
>>>
>>
>> I did try to drop such NOOPs, but then something called
>> sanitycheckcpp.exe failed the build because it required 'extern "C"' in
>> those header files.
>>
>> Isn't that check superfluous? A missing 'extern "C"' would be detected
>> at a later stage, when the dummy C++ programs are compiled against the
>> public header files.
>>
>> If we agree santifycheckcpp.exe should be fixed, is that a separate
>> patch or need it be a part of this patch set?
> 
> This check was added with 1ee492bdc4ff ("buildtools/chkincs: check
> missing C++ guards").
> The check is too naive, and I am not sure we can actually make a better one...
> 
> I would remove this check, if no better option.
> 

Just to be clear: what you are suggesting is removing the check as a 
part of this patch set?

I think I was wrong saying the dummy C++ programs already detect 
omissions of C linkage.

I'll leave for Bruce to comment on this before I do anything.

> 
>>>> diff --git a/lib/eal/include/generic/rte_atomic.h b/lib/eal/include/generic/rte_atomic.h
>>>> index f859707744..0a4f3f8528 100644
>>>> --- a/lib/eal/include/generic/rte_atomic.h
>>>> +++ b/lib/eal/include/generic/rte_atomic.h
>>>> @@ -17,6 +17,10 @@
>>>>    #include <rte_common.h>
>>>>    #include <rte_stdatomic.h>
>>>>
>>>> +#ifdef __cplusplus
>>>> +extern "C" {
>>>> +#endif
>>>> +
>>>>    #ifdef __DOXYGEN__
>>>>
>>>>    /** @name Memory Barrier
>>>> @@ -1156,4 +1160,8 @@ rte_atomic128_cmp_exchange(rte_int128_t *dst,
>>>>
>>>>    #endif /* __DOXYGEN__ */
>>>>
>>>> +#ifdef __cplusplus
>>>> +}
>>>> +#endif
>>>> +
>>>
>>> I would move under #ifdef DOXYGEN.
>>>
>>
>> Why? The pattern now is "almost always directly after the #includes".
>> That is better than before, but not ideal. C linkage should only be
>> covering functions and global variables declared, I think.
> 
> I hear you about how the marking was done but it already has some
> manual edits (seeing how some fixes were needed).
> 
> 

I was not arguing against manual edits. I was arguing against 
inconsistent placement of the #ifdefs.

That said, I don't know what the purpose of the ifdef DOXYGEN is.