Re: [PATCH libdrm] intel: annotate the intel genx helpers as private

[Lucas De Marchi <lucas.demarchi@intel.com>]

On Thu, Sep 06, 2018 at 06:59:33PM +0100, Chris Wilson wrote:
> Quoting Lucas De Marchi (2018-09-06 18:51:37)
> > On Thu, Sep 06, 2018 at 06:38:52PM +0100, Chris Wilson wrote:
> > > Quoting Emil Velikov (2018-09-06 16:14:07)
> > > > From: Emil Velikov <emil.velikov@collabora.com>
> > > > 
> > > > They're used internally and never meant to be part of the API.
> > > > Add the drm_private notation, which should resolve that.
> > > > 
> > > > Cc: Eric Engestrom <eric.engestrom@intel.com>
> > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Fixes: 4e81d4f9c9b ("intel: add generic functions to check PCI ID")
> > > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> > > > ---
> > > >  intel/intel_chipset.c | 4 ++--
> > > >  intel/intel_chipset.h | 4 ++--
> > > >  2 files changed, 4 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/intel/intel_chipset.c b/intel/intel_chipset.c
> > > > index d5c33cc5..5aa4a2f2 100644
> > > > --- a/intel/intel_chipset.c
> > > > +++ b/intel/intel_chipset.c
> > > > @@ -44,7 +44,7 @@ static const struct pci_device {
> > > >         INTEL_SKL_IDS(9),
> > > >  };
> > > >  
> > > > -bool intel_is_genx(unsigned int devid, int gen)
> > > > +drm_private bool intel_is_genx(unsigned int devid, int gen)
> > > >  {
> > > >         const struct pci_device *p,
> > > >                   *pend = pciids + sizeof(pciids) / sizeof(pciids[0]);
> > > > @@ -66,7 +66,7 @@ bool intel_is_genx(unsigned int devid, int gen)
> > > >         return false;
> > > >  }
> > > >  
> > > > -bool intel_get_genx(unsigned int devid, int *gen)
> > > > +drm_private bool intel_get_genx(unsigned int devid, int *gen)
> > > 
> > > We should only need to put the attribute in the header, right?
> > 
> > IMO it actually makes more sense to be in the .c. Reason is that if we
> > are going to change to be hidden by default and annotate the public
> > ones, then we don't need to play with macros to hide them from other
> > projects including the header.
> > 
> > A declaration for a private symbol should not be in an exported header
> > so we know that all the functions in that header should actually be
> > public.
> 
> And we definitely should not be and are not exporting intel_chipset.h.
> 
> I'd would rather have visibility declared in the header because that's
> where I expect interface documentation.

but then if the header is exported, you need to export the definition of
the macro as well.. and undef it when it's not for internal use. I've
seen nasty things on projects that went this route, because then
you not only depend on the compiler version you are compiled with, but
you also need to keep the flexibility for other projects that are
including you. Example:


#ifdef EAPI
# undef EAPI
#endif

#ifdef _WIN32
# ifdef EFL_BUILD
#  ifdef DLL_EXPORT
#   define EAPI __declspec(dllexport)
#  else
#   define EAPI
#  endif
# else
#  define EAPI __declspec(dllimport)
# endif
# define EAPI_WEAK
#else
# ifdef __GNUC__
#  if __GNUC__ >= 4
#   define EAPI __attribute__ ((visibility("default")))
#   define EAPI_WEAK __attribute__ ((weak))
#  else
#   define EAPI
#   define EAPI_WEAK
#  endif
# else
#  define EAPI
#  define EAPI_WEAK
# endif
#endif


Lucas De Marchi
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel