[Mesa-dev] [PATCH 1/7] util: add MAYBE_UNUSED for config dependent variables

[Ian Romanick]

On 04/19/2016 07:48 AM, Emil Velikov wrote:
> On 18 April 2016 at 19:01, Ian Romanick <idr at freedesktop.org> wrote:
>> On 04/18/2016 08:06 AM, Emil Velikov wrote:
>>> On 18 April 2016 at 04:43, Francisco Jerez <currojerez at riseup.net> wrote:
>>>> Grazvydas Ignotas <notasas at gmail.com> writes:
>>>>
>>>>> On Sun, Apr 17, 2016 at 2:50 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>>>> On 16 April 2016 at 02:00, Grazvydas Ignotas <notasas at gmail.com> wrote:
>>>>>>> This is mostly for variables that are only used in asserts and cause
>>>>>>> unused-but-set-variable warnings in release builds. Could just use
>>>>>>> UNUSED directly, but MAYBE_UNUSED should be less confusing and is
>>>>>>> similar to what the Linux kernel has.
>>>>>>>
>>>>>>> And yes __attribute__((unused)) can be used on variables on both GCC 4.2
>>>>>>> (oldest supported by mesa) and clang 3.0 (just some random old version,
>>>>>>> nut sure what's the minimum for mesa).
>>>>>>>
>>>>>>> Signed-off-by: Grazvydas Ignotas <notasas at gmail.com>
>>>>>>> ---
>>>>>>> I have no commit access, if this patch is ok, please someone push.
>>>>>>>
>>>>>>>  src/util/macros.h | 2 ++
>>>>>>>  1 file changed, 2 insertions(+)
>>>>>>>
>>>>>>> diff --git a/src/util/macros.h b/src/util/macros.h
>>>>>>> index 0c8958f..f081bb8 100644
>>>>>>> --- a/src/util/macros.h
>>>>>>> +++ b/src/util/macros.h
>>>>>>> @@ -204,6 +204,8 @@ do {                       \
>>>>>>>  #define UNUSED
>>>>>>>  #endif
>>>>>>>
>>>>>>> +#define MAYBE_UNUSED UNUSED
>>>>>>> +
>>>>>> Hell yeah !
>>>>>>
>>>>>> A thing that comes to mind ... a while back we've been wondering about
>>>>>> (re)naming these just the the way we do in the kernel. Namely
>>>>>> __maybe_unused in this case. Can you give that one a try and link a
>>>>>> branch.
>>>>>
>>>>> I hope you mean something like this?
>>>>> https://github.com/notaz/mesa/commits/warnings
>>>>>
>>>>
>>>> You guys know that in standard C any identifier (including a
>>>> preprocessor define) starting with double underscore is reserved for the
>>>> implementation and causes the program's behavior to be undefined?  The
>>>> kernel is kind of part of the implementation so they can do whatever
>>>> they want, but it seems dubious to do the same in a userspace library.
>>>> How about you use a single (or no) underscore if people prefer the
>>>> lowercase spelling?
>>>>
>>> I do recall this. As mentioned before (to Jose I believe) sadly the
>>> cat is out of the bag. Both within mesa itself and also in other
>>> projects.
>>>
>>> IIRC the conclusion from last time was along the lines of "[I guess]
>>> it's ok if it does not break things". So far scons (linux + windows)
>>> look fine - autoconf's normal make looks ok as well (make check takes
>>> a while) ;-)
>>>
>>> Personally I'd opt for consistency across projects. It gives us extra
>>> reassurance that things are unlikely to break under our feet.
>>> Although yes, it does suck (a bit) that thing have turned out that way.
>>
>> I agree with Curro.  I'd rather have consistency within Mesa (where
>> we've always used ALL_THE_UPPERCASE) than across projects.
>>
> Either I'm loosing the plot or that hasn't been true for a while. Take
> a look at src/util/macros.h and alike.

The only things in macros.h that are related and use lower case are
likely, unlikely, unreachable, and assume.  All four of those are lower
case because they mimic keywords in some compiler.  We don't need a new
spelling (or capitalization) of a name for a thing that already has a
name.  See also my rage against GLvoid, GLint, GLfloat, etc.

At the same time, we have ATTRIBUTE_CONST, FLATTEN, PRINTFLIKE,
MALLOCLIKE, PACKED, ATTRIBUTE_PURE, PUBLIC, USED, UNUSED, and MUST_CHECK.

None of these names have multiple underscores in the name.

Naming this new thing MAYBE_UNUSED (or perhaps MIGHT_BE_UNUSED) seems
pretty obvious to me.  Anything else seems weird.

> That said, I feel that this might turn into bikeshedding pretty quick,
> so if the above is not enough [to change your mind] so be it.
> 
> Emil