[Mesa-dev] [PATCH 06/20] mesa: Move {split, merge}_location_offset to uniforms.h

[Ian Romanick]

On 10/30/2011 10:47 PM, Kenneth Graunke wrote:
> On 10/28/2011 10:42 AM, Ian Romanick wrote:
>> From: Ian Romanick<ian.d.romanick at intel.com>
>>
>> Also prepend _mesa_uniform_ to the names.
>>
>> Signed-off-by: Ian Romanick<ian.d.romanick at intel.com>
> ...
>> -static void
>> -merge_location_offset(GLint *location, GLint offset)
>> -{
>> -   *location = (*location<<  16) | offset;
>> -}
> ...
>> +/**
>> + * Combine the uniform's base location and the offset
>> + */
>> +static inline GLint
>> +_mesa_uniform_merge_location_offset(unsigned base_location, unsigned
> offset)
>> +{
>> +   return (base_location<<  16) | offset;
>> +}
>> +
>
> You've clearly done more than what your commit message says here...you
> changed this function's calling convention from foo(&x,y) to x=foo(x,y)
> and rewrote the callers appropriately.
>
> While I'm not opposed to that, it's worth mentioning.
>
>> -/**
>> - * Separate the uniform location and parameter offset.  See above.
>> - */
>> -static void
>> -split_location_offset(GLint *location, GLint *offset)
>> -{
>> -   *offset = *location&  0xffff;
>> -   *location = *location>>  16;
>> -}
> ...
>> +/**
>> + * Separate the uniform base location and parameter offset
>> + */
>> +static inline void
>> +_mesa_uniform_split_location_offset(GLint location, unsigned *base_location,
>> +				    unsigned *offset)
>> +{
>> +   *offset = location&  0xffff;
>> +   *base_location = location>>  16;
>> +}
>
> This one is even stranger.  You added a third parameter and rewrote the
> existing callers to do:
>
> _mesa_uniform_split_location_offset(location,&location, offset).
>
> This seems rather weird and redundant at first glance.  Though, I think
> I see your rationale: location is an input parameter containing
> base+offset (together), while the next two are output parameters for
> base and offset (separately).  The former function muddled things by
> forcing you to override the combined input, which is a pain for callers
> that want to keep all three.
>
> So I suppose I'm not opposed to this either, but please do
> mention/justify the change in the commit message.

Good call.

I had a couple reasons for the second change, and I'll add them to the 
commit message.

1. Having 'location' in the function mean different things in different 
places was confusing and annoying to me.

2. I seem to recall that a later patch wants to use the original value 
of location sometime after splitting out the index and offset.