[Mesa-dev] Fwd: [PATCH] glcpp: simplify the check for hexadecimal number

[Vlad Golovkin]

---------- Forwarded message ----------
From: Vlad Golovkin <vlad.golovkin.mail at gmail.com>
Date: 2017-06-02 16:55 GMT+03:00
Subject: Re: [Mesa-dev] [PATCH] glcpp: simplify the check for hexadecimal number
To: Emil Velikov <emil.l.velikov at gmail.com>


2017-06-02 16:07 GMT+03:00 Emil Velikov <emil.l.velikov at gmail.com>:
> Hi Vlad,
>
> Welcome to Mesa.
>
> Have you don't any benchmarking on this patch via something like shader-db [1]?
> Mentioning the results in the commit message is encouraged.

Hi Emil,

If I understand correctly shader-db supports only i965 and radeonsi,
or am I missing something?
I would be glad to be proven wrong, because I only have old r600 gpu.

>
> On 2 June 2017 at 13:37, Vlad Golovkin <vlad.golovkin.mail at gmail.com> wrote:
>> This will make it possible to remove unnecessary strlen and strncmp
>> calls.
>> ---
>>  src/compiler/glsl/glcpp/glcpp-parse.y | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/compiler/glsl/glcpp/glcpp-parse.y b/src/compiler/glsl/glcpp/glcpp-parse.y
>> index fe211a0f0b..8cdd4c9d5b 100644
>> --- a/src/compiler/glsl/glcpp/glcpp-parse.y
>> +++ b/src/compiler/glsl/glcpp/glcpp-parse.y
>> @@ -450,10 +450,12 @@ control_line_error:
>>
>>  integer_constant:
>>         INTEGER_STRING {
>> -               if (strlen ($1) >= 3 && strncmp ($1, "0x", 2) == 0) {
>> -                       $$ = strtoll ($1 + 2, NULL, 16);
>> -               } else if ($1[0] == '0') {
>> -                       $$ = strtoll ($1, NULL, 8);
>> +               if ($1[0] == '0') {
>> +                       if ($1[1] == 'x' && $1[2] != '\0') {
>> +                               $$ = strtoll ($1 + 2, NULL, 16);
>> +                       } else {
>> +                               $$ = strtoll ($1, NULL, 8);
>> +                       }
>>                 } else {
>>                         $$ = strtoll ($1, NULL, 10);
>>                 }
> Food for thought:
>
> Any idea why we'd want to handle any of these details ourselves?
> strtoll(..., /*base*/ 0) is smart enough, plus it should be better optimised.
This is a good approach, it will allow to write just one simple
function call instead of all these checks.
For some reason i thought that calling strtoll(str, NULL, 0) will make
it autodetect the base - from 2 to 36, when in reality it basically
tries to match bases 8, 10 and 16.
>
> Another possibility is to swap the INTEGER_STRING token for
> {DEC,OCT,HEX}... ones each one calling strtoll() with correct base.
Advantage of this approach, however, is that in this case one can just
use custom string to num functions that avoid the overhead of strtoll,
but then again it is better to benchmark before drawing any
conclusions.
>
> -Emil
>
> [1] https://cgit.freedesktop.org/mesa/shader-db/