Re: [dpdk-dev] [dpdk-dev, 07/10] lib: fix missing include dependencies

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

Hi Jan,

Replying below as well.

On Tue, Apr 05, 2016 at 10:23:04PM +0200, Jan Viktorin wrote:
> Hello Adrien,
> 
> just quickly skimming through the ARM fixes...
> 
> On Tue,  5 Apr 2016 16:08:07 +0200
> Adrien Mazarguil <adrien.mazarguil@6wind.com> wrote:
> 
> > Exported header files for use by applications should be self sufficient and
> > allow out of order inclusion. Moreover, they must include all the system
> > headers they need for types and macros.
> > 
> > This commit prevents the following errors:
> > 
> >  error: `RTE_MAX_LCORE' undeclared here (not in a function)
> >  error: `RTE_LPM_VALID_EXT_ENTRY_BITMASK' undeclared (first use in this function)
> >  error: #error "Unsupported cache line size"
> >  error: `asm' undeclared (first use in this function)
> >  error: implicit declaration of function `[...]'
> >  error: unknown type name `[...]'
> >  error: field `mac_addr' has incomplete type
> >  error: `CHAR_BIT' undeclared here (not in a function)
> >  error: `struct timespec' declared inside parameter list
> > 
> > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > 
> > ---
> [...]
> > +
> >  #ifdef __cplusplus
> >  extern "C" {
> >  #endif
> > diff --git a/lib/librte_eal/common/include/arch/arm/rte_byteorder.h b/lib/librte_eal/common/include/arch/arm/rte_byteorder.h
> > index 3f2dd1f..c2078e7 100644
> > --- a/lib/librte_eal/common/include/arch/arm/rte_byteorder.h
> > +++ b/lib/librte_eal/common/include/arch/arm/rte_byteorder.h
> > @@ -37,6 +37,9 @@
> >  #  error Platform must be built with CONFIG_RTE_FORCE_INTRINSICS
> >  #endif
> >  
> > +#include <stdint.h>
> > +#include <rte_common.h>
> 
> Why not to place it into the extern "C" { block? There is already:
> 
> #include "generic/rte_byteorder.h"

Right, I did not do it because headers may eventually contain C++
compatibility code someday, so I think we should avoid #includes inside
extern "C" blocks. C++ compliant headers should provide their own blocks,
also I'm not sure how well it mixes with system includes having their own
compatibility layer.

I agree we need consistency, so what about a commit to move all #includes
outside of such blocks instead?

> > +#include <rte_common.h>
> 
> I don't see any reason for this. The header does not use anything
> special. Just "asm", but that should be a keyword...

Unfortunately it's a nonstandard keyword which is defined as __asm__ in
rte_common.h, itself an extension keyword compilers will swallow without
complaining thanks to these "__".

> >  #ifdef __cplusplus
> >  extern "C" {
> >  #endif
> > diff --git a/lib/librte_eal/common/include/arch/arm/rte_prefetch_64.h b/lib/librte_eal/common/include/arch/arm/rte_prefetch_64.h
> > index 3ed46a4..600c6f0 100644
> > --- a/lib/librte_eal/common/include/arch/arm/rte_prefetch_64.h
> > +++ b/lib/librte_eal/common/include/arch/arm/rte_prefetch_64.h
> > @@ -33,6 +33,8 @@
> >  #ifndef _RTE_PREFETCH_ARM_64_H_
> >  #define _RTE_PREFETCH_ARM_64_H_
> >  
> > +#include <rte_common.h>
> 
> Same here.

Same reason here.

-- 
Adrien Mazarguil
6WIND