On Sat, Jan 25, 2014 at 12:57 PM, Carl Worth <cwo...@cworth.org> wrote:
> Matt Turner <matts...@gmail.com> writes:
>> The check was in the wrong place, ...
>
> I don't doubt that the change here is good and correct, but I think
> there's likely some additional renaming that can be applied:
>
>> + if (parser->version_resolved)
>> + return;
>> +
>> parser->version_resolved = true;
>
> Checking this field immediately before setting it clearly looks correct.
>
>> glcpp_parser_resolve_version(glcpp_parser_t *parser)
>> {
>> - if (parser->version_resolved)
>> - return;
>> -
>
> But it looks odd to not be checking the "version_resolved" field within
> the "resolve_version" function.
>
> I'm not looking at the actual code now in order to provide a
> recommendation, but perhaps it would be more clear if the field were
> renamed to be consistent with the naming of the function performing the
> guarded manipulation?
>
> -Carl
Right, that's a good suggestion. I'm not able to come up with a new
name for either the field or function that I'm terribly happy with.
I've renamed
glcpp_parser_resolve_version -> glcpp_parser_resolve_implicit_version
which hopefully makes it a bit clearer? Two new patches on the mailing list.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
