Re: char-ranges.patch

[Pavel Roskin]

Hello, Akim!

> | ac_cr_09 sounds weird. ac_cr_num would be better.
> 
> I tried to keep names which could reflect both the categories such as
> :alnum:, and ranges.  So maybe instead of _09 it should be _digit and
> instead of _az, it should be _lower.  But, otoh, _az and _09 are more
> precisely what we mean than _lower and _digit.  Hence the name.  And
> also, it is shorter.  Yet in different places I've had to break lines.

It was just a hint. No problem if you are strong in your preferences.

> | ac_hostname could belong to a separate patch.
> 
> Yes, it could.  I agree a perfect patch should separate them, but does
> it really matter here?

No. It's just the matter of convenience.

> | Akim, you should be allowed to add newlines without asking in the list :-)
> 
> :)

I really mean it :-)

> | > +# Name of the host.
> | > +# hostname on some systems (SVR3.2, Linux) returns a bogus exit status,
> | > +# so uname gets run too.  Use two double quotes for font-lock.
> | > +ac_hostname=`(hostname || uname -n) 2>/dev/null | sed 1q`
> | 
> | Double quotes??? Font-lock???
> | This part of the comment should probably be left where it stood.
> 
> Heck.  Thanks, sorry about that.

Aha, it was another hint, but this time it turned out to be more useful.

> | > +# Avoid depending upon Character Ranges.
> | 
> | Why capitalized?
> 
> To highlight why variables are named ac_cr_.

Ok.

> | You replaces single quotes with double quotes. Maybe it's safe, but I would
> | feel safer if single quotes were used everywhere except variables:
> | 
> | ac_tr_sh='sed y%*+%pp%;s%[[^_'$ac_cr_alnum']]%_%g'
> 
> I think this is cluttering readability.  I agree there are places
> where the approach you advocate is right, but I tend to think it's
> overkill here.  Now, if everybody insists...

It's sufficient that you realize the danger and scan the line for things
that can possibly behave differently inside double quotes. I don't insist
on anything.

> | > -      # `set' does not quote correctly, so add quotes (double-quote 
> substitution
> | > -      # turns \\\\ into \\, and sed turns \\ into \).
> | > +      # `set' does not quote correctly, so add quotes (double-quote
> | > +      # substitution turns \\\\ into \\, and sed turns \\ into \).
> | 
> | That's great but please do it separately.
> 
> Really!?!?!  Why???

Because you would not have to ask me why :-)

> | >        # `set' quotes correctly as required by POSIX, so do not add 
> quotes.
> | > -      sed -n '[s/^\([a-zA-Z0-9_]*_cv_[a-zA-Z0-9_]*\)=\(.*\)/\1=\2/p]'
> | > +      sed -n \
> | > +        
> ["s/^\\([_$ac_cr_alnum]*_cv_[_$ac_cr_alnum]*\\)=\\(.*\\)/\\1=\\2/p"]
> | 
> | Again single quoting vs. double quoting.
> 
> Yep, so what :)

Just a warning sign.

> | > -pushdef(AC_Prog, translit($1, a-z, A-Z))dnl
> | > +pushdef([AC_Prog], translit([$1], [a-z], [A-Z]))dnl
> | 
> | It's a separate issue again. It's fine with me if you apply such changes 
> without
> | asking anybody.
> 
> Yes, OK, OK, I go ahead and apply this part of the patch now.

Great! Thank you!

> | > -AC_PATH_PROG(AC_Prog, $1)
> | > +AC_PATH_PROG(AC_Prog, [$1])
> | 
> | Why not [AC_Prog] in this case? 
> 
> Because I'm was not sure AC_PATH_PROG quotes properly, so it's just
> for safety.  But you are right, I should have checked and quoted.

Thank you!

> | What has all this to do with character ranges?
> 
> Just the fact that this was on my way and I hate wasting time in
> making zillions of mini patches :(  Bad attitude I guess, but...

No, not bad attitude! Akim, you are doing a great job! Autoconf wouldn't
ever be like it is without you!

Probably you should just find better tools. I have absolutely no problems
making many patches.

-- 
Regards,
Pavel Roskin