Re: several messages

[Jonathan Nieder]

Joel E. Denny wrote:

> If you have time, please do the following:
> 
> 1. Address the comments I've inlined in your patches below.
> 
> 2. Write a ChangeLog entry.  One for all of the changes you posted seems 
> fine.  Please try to follow the format you see in existing ChangeLog 
> entries.

Sure thing.  Thanks for the tips.

> 3. After committing in your local repo, email us the output of:
> 
>   git format-patch --stdout -1
> 
> That will make your patch easier to apply.

I am not sure I understand this one.  Do you mean that I should keep
the "From " mbox delimiter and From:, Date:, etc fields inline in my
message?

> On Sat, 12 Dec 2009, Jonathan Nieder wrote:

>> diff --git a/src/parse-gram.c b/src/parse-gram.c
>> index 4b6d559..0f509e6 100644
>> --- a/src/parse-gram.c
>> +++ b/src/parse-gram.c
>
>> diff --git a/src/parse-gram.h b/src/parse-gram.h
>> index 894b5dd..fcf3255 100644
>> --- a/src/parse-gram.h
>> +++ b/src/parse-gram.h
>
> When posting patches, it's best if you remove all changes that were 
> automatically generated during the build.  We can regenerate those 
> ourselves, and we don't want to read them while reviewing your patches.

That patch consisted only of automatically generated changes.  I’ll
omit it and combine the other two when I reroll.

>> --- a/data/yacc.c
>> +++ b/data/yacc.c
>> @@ -668,7 +668,7 @@ while (YYID (0))
>>     we won't break user code: when these are the locations we know.  */
>>  
>>  #ifndef YY_LOCATION_PRINT
>> -# if YYLTYPE_IS_TRIVIAL
>> +]b4_locations_if([[# if defined YYLTYPE_IS_TRIVIAL && YYLTYPE_IS_TRIVIAL
>>  #  define YY_LOCATION_PRINT(File, Loc)                      \
>>       fprintf (File, "%d.%d-%d.%d",                  \
>>            (Loc).first_line, (Loc).first_column,     \
>> @@ -676,7 +676,9 @@ while (YYID (0))
>>  # else
>>  #  define YY_LOCATION_PRINT(File, Loc) ((void) 0)
>>  # endif
>> -#endif
>> +]], [[ /* This macro is maintained here in case user code is relying on it. 
>> */
>> +# define YY_LOCATION_PRINT(File, Loc) ((void) 0)
>> +]])[#endif
>
> How does the b4_locations_if part of this change help?

Not much, admittedly.  Here is why I did it anyway:

 1. It makes sure that YYLTYPE_IS_TRIVIAL does not get used if
    locations are turned off.  Since that symbol is in yacc’s
    namespace anyway, that is not a big deal.  But every other
    use of YYLTYPE_IS_TRIVIAL is already protected, and for peace
    of mind I think that is worth maintaining.

    In particular, it is a little clearer that this preprocessor
    symbol is not the cause of any problem that appears with
    locations turned off when grepping for YYLTPYE_IS_TRIVIAL in
    the generated parser yields no matches.

 2. It makes the generated y.tab.c simpler if locations are turned
    off.  I do think that making the y.tab.c less mysterious when
    possible is worth while.

 3. It makes what happens in the locations-turned-off case a little
    clearer.  In my opinion, except for compatibility reasons, it
    doesn’t make sense to define the YY_LOCATION_PRINT macro when
    locations are turned off.  Writing the code this way clarifies
    that we are aware we are defining it and why.

On the other hand, it makes the source uglier.  So I’m not sure what
the right thing to do is.

Jonathan