From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from arjuna.pair.com (arjuna.pair.com [209.68.5.131]) by sourceware.org (Postfix) with ESMTPS id EAA32385840B for ; Tue, 5 Oct 2021 16:15:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org EAA32385840B Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=bitrange.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=bitrange.com Received: by arjuna.pair.com (Postfix, from userid 3006) id 916598A52D; Tue, 5 Oct 2021 12:15:58 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by arjuna.pair.com (Postfix) with ESMTP id 90D3F8A4FA; Tue, 5 Oct 2021 12:15:58 -0400 (EDT) Date: Tue, 5 Oct 2021 12:15:58 -0400 (EDT) From: Hans-Peter Nilsson X-X-Sender: hp@arjuna.pair.com To: Hu Jialun cc: gcc-patches@gcc.gnu.org Subject: Re: *PING* [PATCH] libiberty: allow comments in option file In-Reply-To: <20210925125823.33378-1-hujialun@comp.nus.edu.sg> Message-ID: References: <20210925125823.33378-1-hujialun@comp.nus.edu.sg> User-Agent: Alpine 2.20.16 (BSF 172 2016-09-29) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-10.1 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 05 Oct 2021 16:16:00 -0000 On Sat, 25 Sep 2021, Hu Jialun wrote: > Hello, > > Sorry for bumping it again but I guess it was getting overlooked. > > I am very junior with mailing list open source contributions so please feel > free to point out if I have inadvertantly done something in an incorrect way. > > The archive of the original email can be found at > https://gcc.gnu.org/pipermail/gcc-patches/2021-September/579232.html > and is also appended below. > > Best regards, > Hu Jialun This looks useful, I hope a reviewer will eventually notice it. Until then keep pinging (say, weekly)! A few comments (no pun intended): > > > Enables putting line comments into option files included using '@'. > > > > Comments begin with a ';' followed by whitespace, and terminates at the > > end of the line. Backward compability should be satisfactory since ';' > > is a character disallowed in DOS filenames and rarely used in filenames > > or options anywhere else as well. Perhaps mention that the ';' has to be at the start of the line to be considered a comment? Also, while ';' is, as you say, very suitable considering the context, maybe allow the ubiquitous '#' as well? Though, I don't know how common that is in MSWindows and Apple-anything file-paths, in corner cases. Thoughts? > > > > Related discussion: https://gcc.gnu.org/legacy-ml/gcc/2011-02/msg00422.html > > --- > > libiberty/argv.c | 5 +++++ > > libiberty/testsuite/test-expandargv.c | 12 ++++++++++++ > > 2 files changed, 17 insertions(+) > > > > diff --git a/libiberty/argv.c b/libiberty/argv.c > > index 48dcd102461..2bc7569b718 100644 > > --- a/libiberty/argv.c > > +++ b/libiberty/argv.c > > @@ -194,6 +194,11 @@ char **buildargv (const char *input) > > { > > /* Pick off argv[argc] */ > > consume_whitespace (&input); I'd suggest a comment here as to the purpose, e.g. /* Skip comments. */ > > + if (*input == ';') > > + { > > + for (; *input != '\n' && *input != EOS; ++input); The ";" (for the empty block) should be at a line by itself, says the coding-standard. > > + continue; > > + } > > > > if ((maxargc == 0) || (argc >= (maxargc - 1))) > > { > > diff --git a/libiberty/testsuite/test-expandargv.c b/libiberty/testsuite/test-expandargv.c > > index 56c170f9ec6..5640b2b41cf 100644 > > --- a/libiberty/testsuite/test-expandargv.c > > +++ b/libiberty/testsuite/test-expandargv.c > > @@ -142,6 +142,18 @@ const char *test_data[] = { > > "b", > > 0, > > > > + /* Test 7 - Check for comments in option file. */ > > + "abc\n;def\nxy \\;z \\ ;gh", /* Test 7 data */ > > + ARGV0, > > + "@test-expandargv-7.lst", > > + 0, > > + ARGV0, > > + "abc", > > + "xy", > > + ";z", > > + " ;gh", > > + 0, > > + > > 0 /* Test done marker, don't remove. */ > > }; > > > > -- > > 2.33.0 > Thanks and good luck! brgds, H-P