Re: [PATCH v4] Rewrite iconv option parsing [BZ #19519]

[Arjun Shankar <arjun dot is at lostca dot se>]

Hi Carlos,

Thanks for the review

Just about to send a v5, but I wanted to clarify on a couple of your
review comments before I do so:

> > +  gconv_parse_code (&pfc);
> > +  gconv_parse_code (&ptc);
> > +
> 
> Suggest:
> 
> /* We ignore error handlers from the fromcode because that doesn't make
>    any sense since all errors arise from converting the fromcode to the
>    tocode.  Therefore, only tocode error handlers are used in the
>    conversion specifier.  */
> 
> > +  conv_spec->translit = ptc.translit;
> > +  conv_spec->ignore = ptc.ignore;

I was discussing this with Florian and concluded (and I hope he still
agrees) that in reality, it does make sense to accept the IGNORE error
handler in fromcode. IGNORE comes into play when the input has invalid
characters (as per the input character set), and thus it is indeed an option
that seems to logically apply to the "fromcode". The reason we only parse it
from the tocode is because that's what the man page states and is now the
specification. I think we could consider changing this in a forthcoming
patch. It's still a bit odd and deserves a comment, so I have now
added a slighty different comment in the code at this point,
stating that we only parse tocode suffixes because that's what the
specification states. I also added three test cases to tst-iconv-opt.c to
verify that options passed in the fromcode are indeed ignored as claimed.

> > +ICONV='$codir/elf/ld.so --library-path $LIBPATH --inhibit-rpath ${from}.so \
> > +       $codir/iconv/iconv_prog'
> 
> Why use '' here? You can just use "" and it should keep parameters separate.

It is because ${from} hasn't been set yet. It's set at every iteration of
the loop and thus needs to be expanded at that point. I let this bit of code
stay as-is in v5 because of the expansion.

> > +# Requires $twobyte input, $c flag, $from, and $to to be set; sets $ret
> > +execute_test ()
> > +{
> > +  PROG=`eval echo $ICONV`
> 
> Why do you eval echo through a variable here?
> 
> I expect this is due to your use of '' above, remove that and you should
> be able to use $ICONV directly below.

For the same reason, I've left this bit of code as-is in v5.

Once again, thank you for the review. Hoping for v5 to be more acceptable!
I've taken care of all other review comments that you had on v4. Patch
incoming.

Cheers,
Arjun