[Mesa-dev] [RFC 08/21] mesa/extensions: Replace extension::api_set with ::version

[Chad Versace]

On Thu 22 Oct 2015, Matt Turner wrote:
> On Thu, Oct 22, 2015 at 11:24 AM, Chad Versace <chad.versace at intel.com> wrote:
> > On Mon 19 Oct 2015, Nanley Chery wrote:
> >> From: Nanley Chery <nanley.g.chery at intel.com>
> >>
> >> The api_set field has no users outside of _mesa_extension_supported().
> >> Remove it and allow the version field to take its place.
> >>
> >> The brunt of the transformation was performed with the following vim commands:
> >> s/\(GL [^,]\+\),\s*\d*,\s*\d*\(,\s*\d*\)\(,\s*\d*\)/\1, GLL, GLC\2\3/g
> >> s/\(GLL [^,]\+\)\,\s*\d*/\1, GLL/g
> >> s/\(GLC [^,]\+\)\(,\s*\d*\),\s*\d*\(,\s*\d*\)\(,\s*\d*\)/\1\2, GLC\3\4/g
> >> s/\( ES1[^,]*\)\(,\s*\(\w\|\d\)\+\)\(,\s*\(\w\|\d\)\+\),\s*\d*/\1\2\4, ES1/g
> >> s/\( ES2[^,]*\)\(,\s*\(\w\|\d\)\+\)\(,\s*\(\w\|\d\)\+\)\(,\s*\(\w\|\d\)\+\),\s*\d*/\1\2\4\6, ES2/g
> >>
> >> Signed-off-by: Nanley Chery <nanley.g.chery at intel.com>
> >> ---
> >>  src/mesa/main/extensions.c       |  21 +-
> >>  src/mesa/main/extensions_table.h | 636 ++++++++++++++++++++-------------------
> >>  2 files changed, 326 insertions(+), 331 deletions(-)
> >
> > [...]
> >
> >> diff --git a/src/mesa/main/extensions_table.h b/src/mesa/main/extensions_table.h
> >
> > [...]
> >
> >> +#define GLL 0
> >> +#define GLC 0
> >> +#define ES1 0
> >> +#define ES2 0
> >> +#define  x ~0
> >
> >> +EXT(EXT_abgr                                , dummy_true                             , GLL, GLC,  x ,  x , 1995)
> >> +EXT(EXT_bgra                                , dummy_true                             , GLL,  x ,  x ,  x , 1995)
> >> +EXT(EXT_blend_color                         , EXT_blend_color                        , GLL,  x ,  x ,  x , 1995)
> >> +EXT(EXT_blend_equation_separate             , EXT_blend_equation_separate            , GLL, GLC,  x ,  x , 2003)
> >> +EXT(EXT_blend_func_separate                 , EXT_blend_func_separate                , GLL,  x ,  x ,  x , 1999)
> >> +EXT(EXT_discard_framebuffer                 , dummy_true                             ,  x ,  x , ES1, ES2, 2009)
> >> +EXT(EXT_blend_minmax                        , EXT_blend_minmax                       , GLL,  x , ES1, ES2, 1995)
> >
> > I like the idea of this patch. It's a cleanup that needs to happen. But,
> > the little details confuse me.
> >
> > When I see GLL, GLC, ES1, and ES2 as the entries in the version columns,
> > the entries confuse me because I expect the tokens to all have different
> > values. Otherwise, why would the tokens have different names?
> >
> > I think the table would be less confusing if you used the macros
> >   #define x ~0
> >   #define Y 0
> > or
> >   #define n ~0
> >   #define Y 0
> > or
> >   #define n ~0
> >   #define y 0
> > or anything like that. I like the n/Y the best because the letters are
> > mnenomic and the contrast of uppercase to lowercase is easy to see when
> > scanning the table.
> 
> I get what you're saying, but Nanley's code as is allows you to see
> exactly which APIs an extension exists in without looking up the
> column headers.

You make a good point.

> With your approach, a reader would need to know that the columns are
> ordered GL compat, GL core, ES1, ES2, if I've understood your
> suggestion.

You understood my suggestion correctly.

In the giant table contained in brw_surface_formats.c, we solve the
readability problem by inserting a banner comment at semi-regular intervals,
about every 40 lines. Like this:

   SF( x,  x,  x,  x,  x,  x,  Y,  x,  x, R8G8_SSCALED)
   SF( x,  x,  x,  x,  x,  x,  Y,  x,  x, R8G8_USCALED)
/* smpl filt shad CK  RT  AB  VB  SO  color */
   SF( x,  x,  x,  x,  x,  x,  Y,  x,  x, R16_SSCALED)
   SF( x,  x,  x,  x,  x,  x,  Y,  x,  x, R16_USCALED)
   SF(50, 50,  x,  x,  x,  x,  x,  x,  x, P8A8_UNORM_PALETTE0)
   SF(50, 50,  x,  x,  x,  x,  x,  x,  x, P8A8_UNORM_PALETTE1)

But, as you pointed out, Nanley's solution obviates the need for the banner
comment. And banner comments are clumsy. So I drop my suggestion.