[Mesa-dev] [PATCH 08/15] glsl: Switch ast_type_qualifier to a 128-bit bitset.

[Roland Scheidegger]

Not my area of expertise, but sure.
Reviewed-by: Roland Scheidegger <sroland at vmware.com>


Am 27.02.2018 um 20:14 schrieb Francisco Jerez:
> Do you care enough to give me a reviewed-by so I could land it right
> away?
> 
> Roland Scheidegger <sroland at vmware.com> writes:
> 
>> Please don't wait any longer. We really want appveyor (and some of our
>> own build systems) going again...
>>
>> Roland
>>
>> Am 27.02.2018 um 19:58 schrieb Francisco Jerez:
>>> Thanks for testing.  I'm going to land the build fix with your
>>> Tested-by's if nobody raises any concerns in the next 24h.
>>>
>>> "Kyriazis, George" <george.kyriazis at intel.com> writes:
>>>
>>>> It also fixes the errors that I was getting with gcc 5.4.0 with configure build on ubuntu 16.04.
>>>>
>>>> Tested-By: George Kyriazis <george.kyriazis at intel.com<mailto:george.kyriazis at intel.com>>
>>>>
>>>> On Feb 25, 2018, at 6:52 PM, Roland Scheidegger <sroland at vmware.com<mailto:sroland at vmware.com>> wrote:
>>>>
>>>> Am 25.02.2018 um 21:12 schrieb Francisco Jerez:
>>>> Roland Scheidegger <sroland at vmware.com<mailto:sroland at vmware.com>> writes:
>>>>
>>>> Am 25.02.2018 um 03:35 schrieb Francisco Jerez:
>>>> Roland Scheidegger <sroland at vmware.com<mailto:sroland at vmware.com>> writes:
>>>>
>>>> This seems to have broken compilation with some gcc versions (with scons
>>>> build):
>>>>
>>>> In file included from src/compiler/glsl/ast_array_index.cpp:24:0:
>>>> src/compiler/glsl/ast.h:648:16: error: member
>>>> ‘ast_type_qualifier::bitset_t ast_type_qualifier::flags::i’ with
>>>> constructor not allowed in union
>>>>       bitset_t i;
>>>>                ^
>>>>
>>>> Oops...  And the only reason bitset_t has a default constructor was...
>>>> to avoid using another C++11 feature (defaulted member functions).
>>>> Does the attached patch fix the build failure for you?  The cleaner
>>>> alternative would be to define the default constructor of the bitset
>>>> object like 'T() = default', but that would imply dropping support for
>>>> GCC 4.2-4.3 which don't implement the feature...
>>>>
>>>> FWIW the compile error was happening with gcc 4.8 - I didn't see it with
>>>> gcc 5.4.
>>>> (I don't think at vmware we'd care about anything older than gcc 4.4 at
>>>> least but last time someone wanted to bump gcc requirements there were
>>>> still people requiring gcc 4.2.)
>>>>
>>>> The patch compiles albeit there's about two dozen warnings like the
>>>> following:
>>>> glsl/ast_type.cpp: In member function 'bool
>>>> ast_fully_specified_type::has_qualifiers(_mesa_glsl_parse_state*) const':
>>>> glsl/ast_type.cpp:50:67: warning: ISO C++ says that these are ambiguous,
>>>> even though the worst conversion for the first is better than the worst
>>>> conversion for the second: [enabled by default]
>>>>    return (this->qualifier.flags.i & ~subroutine_only.flags.i) != 0;
>>>>                                                                   ^
>>>> In file included from glsl/ast.h:31:0,
>>>>                 from glsl/ast_type.cpp:24:
>>>> ../../src/util/bitset.h:181:7: note: candidate 1: bool operator!=(const
>>>> ast_type_qualifier::bitset_t&, unsigned int)
>>>>       operator!=(const T &b, BITSET_WORD x)                     \
>>>>       ^
>>>> glsl/ast.h:477:4: note: in expansion of macro 'DECLARE_BITSET_T'
>>>>    DECLARE_BITSET_T(bitset_t, 128);
>>>>    ^
>>>> glsl/ast_type.cpp:50:67: note: candidate 2: operator!=(int, int) <built-in>
>>>>    return (this->qualifier.flags.i & ~subroutine_only.flags.i) != 0;
>>>>                                                                   ^
>>>> Roland
>>>>
>>>>
>>>> Ah, yeah, that's because I didn't provide overloads for signed integer
>>>> types, but it should be harmless since the two candidates have the same
>>>> semantics, and should go away with a C++11-capable compiler.  I think
>>>> the attached patch should shut the warnings on older compilers.
>>>>
>>>> Yes, that compiles without warnings (with gcc 4.8)
>>>> Tested-by: Roland Scheidegger <sroland at vmware.com<mailto:sroland at vmware.com>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> src/compiler/glsl/ast.h:648:16: note: unrestricted unions only available
>>>> with -std=c++11 or -std=gnu++11
>>>> scons: *** [build/linux-x86_64-checked/compiler/glsl/ast_array_index.os]
>>>> Error 1
>>>> src/gallium/tests/unit/u_format_test.c: In function ‘main’:
>>>> src/gallium/tests/unit/u_format_test.c:649:44: warning: array subscript
>>>> is above array bounds [-Warray-bounds]
>>>>          unpacked[i][j] = test->unpacked[i][j][1];
>>>>                                            ^
>>>> In file included from src/compiler/glsl/ast_expr.cpp:24:0:
>>>> src/compiler/glsl/ast.h:648:16: error: member
>>>> ‘ast_type_qualifier::bitset_t ast_type_qualifier::flags::i’ with
>>>> constructor not allowed in union
>>>>       bitset_t i;
>>>>                ^
>>>> src/compiler/glsl/ast.h:648:16: note: unrestricted unions only available
>>>> with -std=c++11 or -std=gnu++11
>>>>
>>>> Roland
>>>>
>>>> [...]
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> mesa-dev mailing list
>>>> mesa-dev at lists.freedesktop.org<mailto:mesa-dev at lists.freedesktop.org>
>>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev