Re: [PATCH 3/3] ylwrap: rename header inclusion in generated parsers

[Stefano Lattarini]

On 07/13/2012 01:59 PM, Akim Demaille wrote:
> 
> Le 13 juil. 2012 à 13:43, Stefano Lattarini a écrit :
> 
>> See?  Another thing I had got wrong, given my ignorance and the lack
>> of a proper explanation ;-)
> 
> I also meant that the bug is obvious :)  If you rename files, you
> have to rename files that use them, and that this is in the context
> of Bison or Flex or whatever, is irrelevant with the nature of
> the bug.  Maybe some day the report file (*.output) will refer to the name
> of the other files, in which case of course it is wrong not to
> rename them.
> 
>>>> Hmmm... can '$from' contain sed metacharacters?  Certainly it can contain
>>>> dots; still, that wouldn't cause spurious failures, only possible (albeit
>>>> very unlikely) extra substitutions in "#include" lines; which I agree we
>>>> don't need worry about, due to their very high unlikeliness.  So the code
>>>> above is correct IMO, but it deserves some more comments, so that a future
>>>> reader won't have to repeat my chain of thoughts.
>>>
>>> The code is exactly the same as what was there before, just moved
>>> elsewhere.
>>>
>> A good excuse to improve comments and explanations, no?
>>
>>> But I can add protections, sure.
>>>
>> Oops, I fear you misread my feedback; I agree that extra protections are
>> overkill, I'd just like you to add a brief comments (along my reasoning
>> above) to explain why this is the case.
> 
> I think it is faster to see the protection being installed
> installed of having to read a comment that states why we think
> the protection is not needed.
>
Agreed (apparently you elided the part of my reply when I stated I prefer
your approach of "over-fix the code rather than over-commenting it", at
least in this case ;-)

>>> OK.  So since I was just using the historical conventions, that
>>> used to be ',', that are used in ylwrap, I understand your
>>> request as a need to change all the other patterns, not just
>>> the one I moved.
>>>
>> Oops, no; that should be done in a follow-up patch if you are interested.
>> Sorry for not stating that explicitly.  Please revert this part of the
>> change.
> 
> Including in the part I just moved that you commented in the
> first place?
>
Both way are OK (use 's|||' right away for the new command, or use 's,,,' at
first, and then convert it together with the other usages in the follow-up
patch).  As you prefer.

>>> +# quote_for_sed [STRING]
>>> +# ----------------------
>>> +# Return STRING (or stdin) quoted to be used as a sed pattern.
>>> quote_for_sed ()
>>> {
>>> -  # FIXME: really we should care about more than '.' and '\'.
>>> -  sed -e 's,[\\.],\\&,g'
>>> +  case $# in
>>> +    0) cat;;
>>> +    1) echo "$1";;
>>>
>> We'd be safer using printf, in case "$1" contains '\' characters:
>>
>>   printf '%s\n' "$1"
> 
> OK.  Should I also remove the other occurrences of echo in ylwrap?
>
If you don't mind writing another follow-up, sure, why not?  Thanks.

>> (the Autoconf manual describes several issues with 'echo' in more
>> details).
> 
> I'm just not used with the idea that printf is portable :)
> 
And still, it is more portable than echo these days.  Isn't life
strange?  :-)

Regards,
  Stefano