[Mesa-dev] [PATCH 1/5] utils: automake: remove uncommon $()

[Emil Velikov]

On 13 August 2015 at 16:23, Matt Turner <mattst88 at gmail.com> wrote:
> On Thu, Aug 13, 2015 at 4:31 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> On 7 August 2015 at 19:22, Matt Turner <mattst88 at gmail.com> wrote:
>>> On Fri, Aug 7, 2015 at 11:05 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>> On 21 July 2015 at 13:12, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>>> On 20/07/15 21:25, Chad Versace wrote:
>>>>>> On Fri 17 Jul 2015, Emil Velikov wrote:
>>>>>>> On 17 July 2015 at 19:11, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>>>>>>> On Fri, Jul 17, 2015 at 2:11 PM, Eric Anholt <eric at anholt.net> wrote:
>>>>>>>>> Matt Turner <mattst88 at gmail.com> writes:
>>>>>>>>>
>>>>>>>>>> On Fri, Jul 17, 2015 at 10:17 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>>>>>>>>> Cc: Eric Anholt <eric at anholt.net>
>>>>>>>>>>> Signed-off-by: Emil Velikov <emil.l.velikov at gmail.com>
>>>>>>>>>>> ---
>>>>>>>>>>>  src/util/tests/hash_table/Makefile.am | 3 +--
>>>>>>>>>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/src/util/tests/hash_table/Makefile.am b/src/util/tests/hash_table/Makefile.am
>>>>>>>>>>> index 04a77e3..0c99e7b 100644
>>>>>>>>>>> --- a/src/util/tests/hash_table/Makefile.am
>>>>>>>>>>> +++ b/src/util/tests/hash_table/Makefile.am
>>>>>>>>>>> @@ -38,7 +38,6 @@ TESTS = \
>>>>>>>>>>>         null_destroy \
>>>>>>>>>>>         random_entry \
>>>>>>>>>>>         remove_null \
>>>>>>>>>>> -       replacement \
>>>>>>>>>>> -       $()
>>>>>>>>>>> +       replacement
>>>>>>>>>>
>>>>>>>>>> To get the benefit of $() without some unknown incompatibility, pixman
>>>>>>>>>> uses $(NULL) which of course relies on not having a variable named
>>>>>>>>>> NULL.
>>>>>>>>>>
>>>>>>>>>> I might suggest that instead of removing them, but I'm not much
>>>>>>>>>> opposed to removing them either.
>>>>>>>>>
>>>>>>>>> I do really like having a terminator on these lists.  I find that
>>>>>>>>> without them, I'll end up copy-and-pasting the wrong thing and missing
>>>>>>>>> the trailing backslash on a line.
>>>>>>>>
>>>>>>>> Also makes diffs easier to read since you don't have spurious changes
>>>>>>>> which just add a \ .
>>>>>>
>>>>>> I second Anholt. I prefer the sentinel too, but don't feel too strongly
>>>>>> about it.
>>>>>>
>>>>>> I find that a sentinel helps me avoid making mistakes when adding new
>>>>>> list members or when sorting the lists in $EDITOR.
>>>>>>
>>>>> In case it's not (too) obvious I'm nuking these for consistency sake, as
>>>>> there are five of these in over 80 makefiles. If people like/prefer them
>>>>> can we have a volunteer that adds them everywhere ?
>>>>>
>>>> Can we have some volunteers, please ?
>>>
>>> I don't think we should require consistency here.
>>>
>> Genuine question - what does this case makes it different from any other ?
>
> I think your question is "why shouldn't we require consistency?"
>
> There are many things in the code base that don't match with our
> current style. We fix them when we modify the code, but otherwise
> don't force changes just for the sake of style. For instance, we don't
> use tabs for indentation but we haven't removed all the tabs either. I
> think this situation is the same.
>
While I agree with the argument yet there is a small catch between the
two. On one side we ~200 diff stat for the whole project, while on the
other hand the diff is in the 1000s if not 10000s. I believe the
latter (plus git blame) is the core reason why the code style hasn't
been unified.

>> We do config/build check before pushing which easily catches the
>> problem of missing \. Do we not ?
>
> That's only partially the point of $(NULL). It's purpose is to avoid
> mistakes,

> but it also allows you to sort the list very easily,
This one also applies for non $(NULL) terminated lists.

> and
> using it means that the tail of the list isn't changed when new items
> are appended (reduces git blame noise).
I believe that I've used git blame less than a dozen times while
working with the build system. So I wouldn't put too much significance
on it.

Either way, it was an interesting discussion, considering the amount
of code affected and its significance. Peace :)

-Emil
P.S. Most editors nicely highlight the code if \ is missing or added
inappropriately.