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
