[Mesa-dev] [PATCH] glsl/glcpp: Handle hex constants with 0X prefix

[Vlad Golovkin]

2018-04-23 3:53 GMT+03:00 Timothy Arceri <tarceri at itsqueeze.com>:
>
>
> On 20/04/18 06:08, Vlad Golovkin wrote:
>>
>> GLSL 4.6 spec describes hex constant as:
>>
>> hexadecimal-constant:
>>      0x hexadecimal-digit
>>      0X hexadecimal-digit
>>      hexadecimal-constant hexadecimal-digit
>>
>> Right now if you have a shader with the following structure:
>>
>>      #if 0X1 // or any hex number with the 0X prefix
>>      // some code
>>      #endif
>>
>> the code between #if and #endif gets removed because the checking is
>> performed
>> only for "0x" prefix which results in strtoll being called with the base 8
>> and
>> after encountering the 'X' char the strtoll returns 0. Letting strtoll
>> detect
>> the base makes this limitation go away and also makes code easier to read.
>
>
> The problem is strtoll supports much more than what GLSL allows.
>
>>
>> Also added 1 test for uppercase hex prefix.
>> ---
>>   src/compiler/glsl/glcpp/glcpp-parse.y                            | 9
>> ++-------
>>   src/compiler/glsl/glcpp/tests/149-hex-const-uppercase-prefix.c   | 5
>> +++++
>>   .../glsl/glcpp/tests/149-hex-const-uppercase-prefix.c.expected   | 5
>> +++++
>>   3 files changed, 12 insertions(+), 7 deletions(-)
>>   create mode 100644
>> src/compiler/glsl/glcpp/tests/149-hex-const-uppercase-prefix.c
>>   create mode 100644
>> src/compiler/glsl/glcpp/tests/149-hex-const-uppercase-prefix.c.expected
>>
>> diff --git a/src/compiler/glsl/glcpp/glcpp-parse.y
>> b/src/compiler/glsl/glcpp/glcpp-parse.y
>> index ccb3aa18d3..d83f99f1c7 100644
>> --- a/src/compiler/glsl/glcpp/glcpp-parse.y
>> +++ b/src/compiler/glsl/glcpp/glcpp-parse.y
>> @@ -462,13 +462,8 @@ control_line_error:
>>     integer_constant:
>>         INTEGER_STRING {
>> -               if (strlen ($1) >= 3 && strncmp ($1, "0x", 2) == 0) {
>
>
>
> As per my coment above strtoll supports much more than what GLSL allows.
> Please just add strncmp($1, "0X", 2) == 0 to the if above.

Flex and Bison is not my strongest suit, so correct me if I am wrong,
but it seems that INTEGER_STRING can't contain leading whitespace
and/or leading plus/minus that strtoll supports.
As for the multitude of bases that strtoll supports, it would ignore
them and just choose between 8, 10 and 16 when base = 0. I think I
should have made it more clear in the code comment.

>
> That patch would have my r-b. If you send that fixed up patch I can push it
> for you. Thanks for looking into this.
>
>
>
>> -                       $$ = strtoll ($1 + 2, NULL, 16);
>> -               } else if ($1[0] == '0') {
>> -                       $$ = strtoll ($1, NULL, 8);
>> -               } else {
>> -                       $$ = strtoll ($1, NULL, 10);
>> -               }
>> +               /* let strtoll detect the base */
>> +               $$ = strtoll ($1, NULL, 0);
>>         }
>>   |     INTEGER {
>>                 $$ = $1;
>> diff --git
>> a/src/compiler/glsl/glcpp/tests/149-hex-const-uppercase-prefix.c
>> b/src/compiler/glsl/glcpp/tests/149-hex-const-uppercase-prefix.c
>> new file mode 100644
>> index 0000000000..1be9b28eb7
>> --- /dev/null
>> +++ b/src/compiler/glsl/glcpp/tests/149-hex-const-uppercase-prefix.c
>> @@ -0,0 +1,5 @@
>> +#if 0x1234abcd == 0X1234abcd
>> +success
>> +#else
>> +failure
>> +#endif
>> diff --git
>> a/src/compiler/glsl/glcpp/tests/149-hex-const-uppercase-prefix.c.expected
>> b/src/compiler/glsl/glcpp/tests/149-hex-const-uppercase-prefix.c.expected
>> new file mode 100644
>> index 0000000000..4cf250f6bb
>> --- /dev/null
>> +++
>> b/src/compiler/glsl/glcpp/tests/149-hex-const-uppercase-prefix.c.expected
>> @@ -0,0 +1,5 @@
>> +
>> +success
>> +
>> +
>> +
>>
>