From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16996 invoked by alias); 3 Aug 2011 17:45:52 -0000 Received: (qmail 16988 invoked by uid 22791); 3 Aug 2011 17:45:50 -0000 X-SWARE-Spam-Status: No, hits=-7.3 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 03 Aug 2011 17:45:24 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p73HirCp006482 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 3 Aug 2011 13:44:53 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p73Hirdl020973; Wed, 3 Aug 2011 13:44:53 -0400 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id p73Hiotd028806; Wed, 3 Aug 2011 13:44:50 -0400 From: Tom Tromey To: iam ahal Cc: gdb-patches@sourceware.org, eliz@gnu.org, pmuldoon@redhat.com, brobecker@adacore.com, pedro@codesourcery.com, drow@false.org, jan.kratochvil@redhat.com Subject: Re: [patch] GDB 7.2: new feature for "backtrace" that cuts path to file (remain filename) References: <20110627160029.GF20676@adacore.com> <834o33qlm9.fsf@gnu.org> <83bowq6x7f.fsf@gnu.org> Date: Wed, 03 Aug 2011 17:45:00 -0000 In-Reply-To: (iam ahal's message of "Tue, 2 Aug 2011 23:41:17 +0400") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.0.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain 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 X-SW-Source: 2011-08/txt/msg00050.txt.bz2 >>>>> "Eldar" == iam ahal writes: Tom> I forget (I never keep records of this, I think perhaps I should) -- did Tom> we get you started on the copyright assignment paperwork? Eldar> Not yet, because my previous patches is very different. I mean the Eldar> different files was changed and I don't know which files I need to Eldar> change in the next patch. Eldar> I hope some people here can accept implementation in this last patch. Eldar> After I can start the copyright assignment. It is best to start early. Sometimes it can take quite a while, and in the meantime your patch will not go in. The list of files does not really matter, since the assignment is for past and future changes. You can ask the copyright clerk for clarification on this point if you need it. I think the patch is basically fine, just a few nits. Eldar> +static int backtrace_skip_compile; Eldar> +static void Tom> Newline between these two lines. Eldar> I'm not sure that's right because I see that there's no newline in Eldar> this case (if you look at other places related 'set backtrace ...'). Yeah, I think a newline is better, though. Eldar> +char * Eldar> +get_display_filename_from_sal (struct symtab_and_line *sal) Tom> Tom> Should have an introductory comment before this function. Eldar> Comment was added in 'frame.h' Ok, thanks. Add a comment saying to see the documentation in frame.h. Eldar> +static const char filename_display_without_comp_dir[] = "without-compile-dir"; I think spelling out "directory" would be better. Eldar> + const char *filename = sal->symtab->filename; Eldar> + const char *dirname = sal->symtab->dirname; Eldar> + size_t flen = strlen (filename); Eldar> + size_t dlen = strlen (dirname); This will crash if filename==NULL or dirname==NULL. Eldar> + if (filename_display_string == filename_display_without_comp_dir Eldar> + && filename && dirname && dlen <= flen Eldar> + && !FILENAME_NCMP (filename, dirname, dlen)) This test is not correct, in that it will match "/tmp/x" with "/tmp/xaaa". I think it needs an additional check for a separator. Eldar> diff -rup gdb-7.2-orig/include/filenames.h gdb-7.2/include/filenames.h Changes to libiberty have to go to gcc-patches. I don't think you actually need this change, though, so it would be simpler and quicker if you left it out. Eldar> +extern int filename_ncmp (const char *s1, const char *s2, size_t n); This is already declared in the trunk filenames.h. Be sure to always write patches against the trunk, not something older. Tom