[Mesa-dev] [PATCH 08/10] mesa: Use VERT_ATTRIB_* indexed array in gl_array_object

[Brian Paul]

On 11/23/2011 11:56 PM, Mathias Fröhlich wrote:
>
> Hi Brian,
>
> Thanks for looking into the changes.
>
> On Wednesday, November 23, 2011 17:24:23 Brian Paul wrote:
>> I think there's a few places where we need to map GLenums for vertex
>> arrays to VERT_ATTRIB_x values.  Maybe there should be a helper
>> function for that:
>>
>> gl_vert_attrib
>> array_enum_to_vert_attrib(GLenum vertexArray)
>> {
>>      switch (vertexArray) {
>>      case GL_VERTEX_ARRAY:
>>         return VERT_ATTRIB_POS;
>>      case GL_VERTEX_COLOR:
>>         return VERT_ATTRIB_COLOR0;
>>      ...
>> }
>>
>> Then the code for all those switch cases could be collapsed together.
>
> I agree that this is a good utility function to have.
>
> The motivation of this particular change is to be as mechanic as possible.
> This change is already huge enough and provides plenty of opportunities to
> break something.
>
> So if you don't mind I would prefer to keep this change seperate from a
> possible followup providing and using the above utility function.
>
>>> -   struct gl_client_array VertexAttrib[MAX_VERTEX_GENERIC_ATTRIBS];
>>> +   /** Vertex attribute arrays */
>>> +   struct gl_client_array VertexAttrib[VERT_ATTRIB_MAX];
>>
>> At some point it might be nice to rename VertexAttrib to simply be
>> Array.  It would be a bit more concise.
>
> Ok, I could easily fold that into this current change the total amount of
> changes is hardly the same.
> In fact during implementation of this patch set, I had that variable named
> differently to make sure I really get all transitions right by giving the
> compiler a chance to point me to any old, different indexed use of this
> variable.
> Hmm, just one thing before I really apply this change to my tree: It's also
> easier to grep for VertexAttrib and find meaningful answers than grep for Array
> and find just the same answers - even with \<Array\>  you are flooded.
> So, if you tell me that you don't care for grep the updated patch contains
> this change.

OK, let's leave the name as-is for the time being.

-Brian