[Mesa-dev] [PATCH 05/13] vbo: Implement method to track the inputs array.

[Mathias Fröhlich]

Hi Brian,

Thanks for the review so far!
Two comments/questions below:

On Friday, 16 February 2018 00:27:16 CET Brian Paul wrote:
> > +/**
> > + * Initialize inputs.
> > + */
> > +void
> > +_vbo_array_init(struct vbo_inputs *inputs);
> > +
> > +
> > +/**
> > + * Update the gl_vertex_array array inside the vbo_inputs structure
> > + * provided the current _VPMode, the provided vao and
> > + * the vao's enabled arrays filtered by the filter bitmask.
> > + */
> > +void
> > +_vbo_update_inputs(struct gl_context *ctx, struct vbo_inputs *inputs);
> 
> If that struct and the two prototypes aren't going to be used outside 
> the VBO module, they should go into vbo_private.h

Well, currently yes.
But in a later step I plan to push the gl_vertex_array[] thing into the 
backend drivers, switch over the individual drivers to direct _DrawVAO use and 
fade out that vbo_inputs struct for most drivers. Remember the binding 
information present in the VAO struct shall be reused without rescanning the 
inputs where they stem from.
Consequently, the tool functions here will be called from those individual 
driver that will still use the gl_vertex_array struct and those drivers need a 
public entry function. That is also the reason why there is a seperate init 
function for this almost trivial currently vbo module internal task. The init 
function would also be of public use in that later step. Finally that is even 
the reason for the seperate 'struct vbo_inputs' that could be as well open 
coded in the vbo module. But pushing that into driver backends is much more 
easy when this is a seperate tool struct with methods on it.

So if you really require me to make them private now I can move them now, but 
a later patch will move them back here or somewhere similar.


> > +   /* The rest must be current inputs. */
> > +   update_current_inputs(ctx, inputs, ~enable & VERT_BIT_ALL);
> 
> The & VERT_BIT_ALL isn't really needed, is it?  I guess there's no harm 
> though.

Currently this is a logical noop and I think the compiler detects that.
Originally the code stems from the time where the mask was 33 bits and there 
it was required. In the case we do further play with the VERT_ATTRIB_* list 
and VERT_BITs it may again be required an I like to be prepared for that, 
especially if its that easy.

I will take care of the rest of the comments!

best

Mathias