[Mesa-dev] [PATCH] fix gcc 8 parenthesis warning

[Timothy Arceri]

On 11/04/18 12:13, Timothy Arceri wrote:
> On 24/03/18 02:55, Eric Engestrom wrote:
>> On Friday, 2018-03-23 08:09:46 -0700, Ian Romanick wrote:
>>> On 03/23/2018 03:52 AM, Eric Engestrom wrote:
>>>> On Friday, 2018-03-23 11:01:23 +0100, Marc Dietrich wrote:
>>>>> fixes warnings like this:
>>>>> [184/1137] Compiling C++ object 
>>>>> 'src/compiler/glsl/glsl at sta/lower_jumps.cpp.o'.
>>>>> In file included from ../src/mesa/main/mtypes.h:48,
>>>>>                   from ../src/compiler/glsl_types.h:149,
>>>>>                   from ../src/compiler/glsl/lower_jumps.cpp:59:
>>>>> ../src/compiler/glsl/lower_jumps.cpp: In member function 
>>>>> '{anonymous}::block_record 
>>>>> {anonymous}::ir_lower_jumps_visitor::visit_block(exec_list*)':
>>>>> ../src/compiler/glsl/list.h:650:17: warning: unnecessary 
>>>>> parentheses in declaration of 'node' [-Wparentheses]
>>>>
>>>> This is gonna be a *very* annoying warning...
>>>>
>>>>>      for (__type *(__inst) = (__type *)(__list)->head_sentinel.next; \
>>>>
>>>> These parentheses are here for a reason: to make sure we can't pass in
>>>> something that would break the code or give it an unexpected behaviour.
>>>
>>> This is usually to avoid things with side effects.  Would anything with
>>> a side-effect even compile here?  Or is there some other case that I'm
>>> missing?
>>
>> I can't think of anything right now off the top of my head, so I'll
>> leave my vote blank for now.
> 
> I've switched to Fedora 28 on one of my machines and this warning is 
> very annoying because I pops up everywhere the list is used. If there 
> are no objections I'm going to push this patch in a couple of days.

Also for what its worth if we forced a minimum version of GCC 4.4 with 
mesa we could use #pragma GCC diagnostic ignored "-Wparentheses" for 
this type of thing. Unfortunately we seem forever stuck on 4.2 due to 
the decisions of one OS. Prior to 4.4 the pragma can only be applied at 
the function level.

[1] 
https://cgit.freedesktop.org/mesa/mesa/commit/?id=370e356ebab4885fc19b2b1d1de2816b6cd4dfc8 



> 
> 
>>
>>>
>>>> I would be inclined to NAK this patch and request we kill this warning
>>>> at build system level instead.
>>>> Shame when compilers self-sabotage like that :/
>>>
>>> This is very annoying because -Wparentheses gives useful warnings when
>>> you don't have () but should. :(
>>
>> I know, this is what I mean by self-sabotage: they introduce a useful
>> warning, then later add a bunch of spam to it for arguably no reason,
>> forcing devs to turn it off to be able to still see the other warnings.
>>
>> IMO the new warning can be useful, but should've been off by default
>> (ie. not in -Wall) and turned on via a new flag, instead of hijacking
>> the existing one.
>>
>>>
>>>>>                   ^
>>>>> ../src/compiler/glsl/lower_jumps.cpp:510:7: note: in expansion of 
>>>>> macro 'foreach_in_list'
>>>>>         foreach_in_list(ir_instruction, node, list) {
>>>>>         ^~~~~~~~~~~~~~~
>>>>>
>>>>> Signed-off-by: Marc Dietrich <marvin24 at gmx.de>
>>>>> ---
>>>>>   src/compiler/glsl/list.h | 4 ++--
>>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/src/compiler/glsl/list.h b/src/compiler/glsl/list.h
>>>>> index f77fe12991..2bfa273554 100644
>>>>> --- a/src/compiler/glsl/list.h
>>>>> +++ b/src/compiler/glsl/list.h
>>>>> @@ -647,12 +647,12 @@ inline void 
>>>>> exec_node::insert_before(exec_list *before)
>>>>>   #endif
>>>>>   #define foreach_in_list(__type, __inst, __list)      \
>>>>> -   for (__type *(__inst) = (__type *)(__list)->head_sentinel.next; \
>>>>> +   for (__type *__inst = (__type *)(__list)->head_sentinel.next; \
>>>>>           !(__inst)->is_tail_sentinel();               \
>>>>>           (__inst) = (__type *)(__inst)->next)
>>>>>   #define foreach_in_list_reverse(__type, __inst, __list)   \
>>>>> -   for (__type *(__inst) = (__type *)(__list)->tail_sentinel.prev; \
>>>>> +   for (__type *__inst = (__type *)(__list)->tail_sentinel.prev; \
>>>>>           !(__inst)->is_head_sentinel();                    \
>>>>>           (__inst) = (__type *)(__inst)->prev)
>>>>> -- 
>>>>> 2.16.2
>>>>>
>>>>> _______________________________________________
>>>>> mesa-dev mailing list
>>>>> mesa-dev at lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>>> _______________________________________________
>>>> mesa-dev mailing list
>>>> mesa-dev at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>>>
>>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev