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

[Bruce Richardson <bruce.richardson@intel.com>]

On Wed, Sep 18, 2024 at 02:09:26PM +0200, Mattias Rönnblom wrote:
> On 2024-09-18 13:15, David Marchand wrote:
> > On Tue, Sep 17, 2024 at 11:30 AM Mattias Rönnblom <hofors@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@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.
> 

I agree that the existing check is very naive. Maybe we can go with a
simple fix like adding an allowlist of files which we ignore for 'extern C'
checking?

I don't remember the details of the original patch unfortunately, but from the
commit log I think I found that just compiling C++ with the C headers
didn't throw any errors for the missing extern. I think the functions need
to be actually called and then attempted linked for us to see the errors,
and that is not something that is easily implemented.

/Bruce