[Mesa-dev] [PATCH 3/3] mesa: Fix backward compatbility for XML parser

[Nicolai Hähnle]

On 17.08.2017 04:20, QuRyu wrote:
> After changing the type of drirc values, the parser will be unable to
> recognize xml files before the change. To achieve backward compatbility,

Spelling: compatibility

The commit message is good, though I would say "... will be unable to 
recognize xml files *from* before the change...". I believe that makes 
it a bit easier to read.

Some more comments about the code itself inline below.


> the parser is relaxed to recognize boolean type options with enum values.
> ---
>   src/util/xmlconfig.c | 22 ++++++++++++++++++++--
>   1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/src/util/xmlconfig.c b/src/util/xmlconfig.c
> index d3f47ec..2999f24 100644
> --- a/src/util/xmlconfig.c
> +++ b/src/util/xmlconfig.c
> @@ -317,8 +317,26 @@ parseValue(driOptionValue *v, driOptionType type, const XML_Char *string)
>               v->_bool = true;
>               tail = string + 4;
>           }
> -        else
> -            return false;
> +        else {
> +            /** Some drirc options, such as pp_shalde, were formerly enum values.
> +             *  Now that they have been turned into boolean values, to achieve
> +             *  backward compatbility relax the check here a little bit */

Good comment, but it should start with a plain /*, i.e. no double-stars. 
The double-stars are used for Doxygen comments only (they can be used to 
generate documentation of functions, variables, etc.).

Also, fix the spelling of pp_celshade and compatibility.


> +            XML_Char *start = string;
> +            int value = strToI(string, &tail, 0);
> +            if (tail == start) {
> +                /* no enum value found  */
> +                string = start;
> +                return false;
> +            } else {
> +                if (value == 1)
> +                    v->_bool = true;
> +                else if (value == 0)
> +                    v->_bool = false;
> +                else
> +                    return false; /* wrong value here */
> +            }

I believe you can (and therefore should ;-)) simplify this piece of 
code, because there is code after the switch statement which does all 
the required error checking. Basically, condensing the code to something 
like:

   int value = strToI(string, &tail, 0);
   if (value == 1)
     v->_bool = true;
   else if (value == 0)
     v->_bool = false;
   else
     return false;

should be enough. All the other error conditions, like empty string, or 
garbage after the tail, are handled below the switch statement.

Cheers,
Nicolai

> +	}
> +
>           break;
>         case DRI_ENUM: /* enum is just a special integer */
>         case DRI_INT:
> 


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.