From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31681 invoked by alias); 9 Dec 2013 09:02:46 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 31670 invoked by uid 89); 9 Dec 2013 09:02:46 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.9 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-qa0-f45.google.com Received: from Unknown (HELO mail-qa0-f45.google.com) (209.85.216.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Mon, 09 Dec 2013 09:02:42 +0000 Received: by mail-qa0-f45.google.com with SMTP id o15so2412217qap.4 for ; Mon, 09 Dec 2013 01:02:34 -0800 (PST) MIME-Version: 1.0 X-Received: by 10.49.129.38 with SMTP id nt6mr23511438qeb.78.1386579754851; Mon, 09 Dec 2013 01:02:34 -0800 (PST) Received: by 10.96.175.66 with HTTP; Mon, 9 Dec 2013 01:02:34 -0800 (PST) In-Reply-To: <20131209083216.GB4011@adacore.com> References: <1386538152-13461-1-git-send-email-a3at.mail@gmail.com> <20131209083216.GB4011@adacore.com> Date: Mon, 09 Dec 2013 09:02:00 -0000 Message-ID: Subject: Re: [PATCH] gdb: set filename-display shortpath support From: Azat Khuzhin To: Joel Brobecker Cc: gdb-patches@sourceware.org Content-Type: text/plain; charset=UTF-8 X-SW-Source: 2013-12/txt/msg00323.txt.bz2 On Mon, Dec 9, 2013 at 12:32 PM, Joel Brobecker wrote: > Hello, > > Thanks for the patch. I've only had a chance to scan the patch, so > cannot comment deeply. But I can make a few comments... Thanks for you review. I resent new patch with coding style issues resolved. (V2) > >> +2013-12-00 Azat Khuzhin >> + >> + * source.h (symtab_to_shortpath): Add it. >> + * source.c (filename-display): Add shortpath display. >> + * source.c (symtab_to_filename_for_display): Use symtab_to_shortpath. > > You don't need to repeat the source filename. In your case, the last > 2 lines of the CL become: > > * source.c (filename_display): Add shortpath display. > (symtab_to_filename_for_display): Use symtab_to_shortpath. > > Note also the '-' incorrectly used instead of '_' in filename_display. > > One small request, also. It's customary to provide the ChangeLog > as text, rather than make it a ChagneLog diff. If you take a look > at a few random messages on this list, you'll probably understand. > ChangeLog files are always getting new changes, and always at the > same location, thus constantly generating sources of conflicts. > Unless you have an automated conflict solver in your setup, it's > fine to send the diff with the ChangeLog in the revision history, > and then only add the entry just before pushing the change to > our repository. Tom also posted some tips on how he was managing > the ChangeLog entries (search for "ChangeLog management tip" in > the gdb@sourceware.org mailing list archives, Oct 25th 2013). > > Overall, I don't remember if other commented, but this sounds like > a useful idea. > >> +#undef MIN >> +#define MIN(A, B) (((A) <= (B)) ? (A) : (B)) > > You don't need that. Use "min", defined in defs.h if not already > defined. I searched by MIN (case-sensitive) that's why I didn't find it. Thanks. > >> +const char * >> +symtab_to_shortpath (struct symtab *symtab) > > Can you add a small comment explaining that the description of that > function is in source.h? The usual form is something minimilistic > such as: > > /* See source.h. */ Forgot that. Thanks. > > It just allows us to quickly know that this function is expected to be > documented elsewhere. > >> + char *prev_slash_name = (char *)symtab->filename; >> + char *prev_slash_dir = (char *)symtab->dirname; >> + char *slash_name = (char *)symtab->filename; >> + char *slash_dir = (char *)symtab->dirname; >> + const char *shortpath = slash_name; > > Formatting nit: space after the ')'. > >> + if (!slash_dir) >> + return shortpath; > > A recent Coding Style decision requires us to explicitly test against > NULL -> if (slash_dir != NULL) > >> + >> + while ((slash_name = strstr(slash_name, SLASH_STRING)) && >> + (slash_dir = strstr(slash_dir, SLASH_STRING))) > > Formatting nit: binary operators should be at the start of the next > line, not at the end. Also, make sure to have a space before '(' > in function calls. There are other such violations in the rest of > the code, which I will not repeat - can you fix the rest as well? > > But we also frown on assignments inside conditions. Can you rewrite > the code to avoid that? >> + { >> + slash_name++; >> + slash_dir++; >> + >> + if (strncmp(slash_name, slash_dir, MIN(slash_name - prev_slash_name, slash_dir - prev_slash_dir))) > > This should should be split to not exceed 80 chars (we like a soft limit > of 70 characters, as long as practical). > >> + basename - display only basename of a filename\n\ >> + relative - display a filename relative to the compilation directory\n\ >> + absolute - display an absolute filename\n\ >> + shortpath - display only non-common part of filename and compilation directory\n\ > > This line is a little bit too long. It's a bit of PIA to be breaking > it; I think we should do it, for the sake of consistency, but it's not > a strong opinion. I think that there is no need in new line for breaking, so I just break it in code. > > -- > Joel -- Respectfully Azat Khuzhin