From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-4.0 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL,SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id 3D6DF1F45E for ; Mon, 10 Feb 2020 19:53:20 +0000 (UTC) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:cc:subject:message-id:references :mime-version:content-type:in-reply-to; q=dns; s=default; b=IwCs g/QeARscIbqMqSZZbG1xuw6IWCDA9MKB4qMaCekBlZLLHwOp4t+nhKMOr3RrgpOl m5GHIXOFE3/aR1b3kYr9hQ6u31PGBrTIXyk5n0oBK0r7UG7gSR2pHVjyQHXeILzh uX28M3dEuWmmeyw5YbsqS1KuL8ldxVTo5j1HIhw= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:cc:subject:message-id:references :mime-version:content-type:in-reply-to; s=default; bh=P5wvvxQt7h wa5wnt1h3Pg29nmV4=; b=aG8ECRyjjbaTj7FdCmEB00hwkRggtN7NNDMK9Sy/kZ mvCy1MgafRi+plh7l4kBSmYTElu8GDjY2tq4bQLEiG0QufzkgWEikCwQJdPOLlzi aDZQcGfRSHoyaLh92lpuB8/GGx88e4scQU1F+DXjjzkUNX1f/JpI24LmjGzjv5rR U= Received: (qmail 116681 invoked by alias); 10 Feb 2020 19:53:17 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 116673 invoked by uid 89); 10 Feb 2020 19:53:17 -0000 Authentication-Results: sourceware.org; auth=none X-HELO: aloka.lostca.se DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=lostca.se; h=date:from:to :cc:subject:message-id:references:mime-version:content-type :in-reply-to; s=howrah; bh=P5wvvxQt7hwa5wnt1h3Pg29nmV4=; b=qeqj5 N42Spq+HcfohCvRAUMG0tAd0KxIlrk2T9Ssk1bRn00CpLKYM852TGc0HaJm+pVCk hkTMKk3rRGZjihy2foVqRx4//Hn9aR1nNL3qp+9rhqw2Fk5phR5MbcSIrlfAqnlX /o7PkS40hrkipoNlzf2Xhf9bFwA7gnpJ2YJY6Q= Date: Mon, 10 Feb 2020 19:53:11 +0000 From: Arjun Shankar To: Carlos O'Donell Cc: libc-alpha@sourceware.org, Siddhesh Poyarekar , Florian Weimer Subject: Re: [PATCH v4] Rewrite iconv option parsing [BZ #19519] Message-ID: <20200210195311.GA171@aloka.lostca.se> References: <20200116155550.GA99449@aloka.lostca.se> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) 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