From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14932 invoked by alias); 9 Dec 2013 08:32:29 -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 14922 invoked by uid 89); 9 Dec 2013 08:32:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.3.2 X-HELO: rock.gnat.com Received: from Unknown (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Mon, 09 Dec 2013 08:32:26 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 25CE41163E0; Mon, 9 Dec 2013 03:32:57 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id fpKgt0eIcfLa; Mon, 9 Dec 2013 03:32:57 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id EC865116167; Mon, 9 Dec 2013 03:32:56 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id DDA1CE16B7; Mon, 9 Dec 2013 09:32:16 +0100 (CET) Date: Mon, 09 Dec 2013 08:32:00 -0000 From: Joel Brobecker To: Azat Khuzhin Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] gdb: set filename-display shortpath support Message-ID: <20131209083216.GB4011@adacore.com> References: <1386538152-13461-1-git-send-email-a3at.mail@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1386538152-13461-1-git-send-email-a3at.mail@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2013-12/txt/msg00320.txt.bz2 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... > +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. > +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. */ 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. -- Joel