Re: [patch] PR 31754/C++ - make -fshow-column produce more accurate column numbers

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Dodji" == Dodji Seketeli <dseketel@redhat.com> writes:

Dodji> This patch tries to fix PR 31754 for the c++ front end.
Dodji> It modifies the c++ parser to produce more accurate column information
Dodji> in error messages.

I skimmed this a bit.  I have a few nits... these are nothing serious,
just the usual tiny details that everybody misses their first time
hacking no GCC :)

A lot of the patch looks straightforward at first glance.  I didn't
look at the error calls in detail; but I note that we applied a
similar treatment to the C parser recently.  (Note that I can't
approve or reject this patch in any case.)

Dodji>     	* cp-tree.h, pt.c, semantic.c:
Dodji>     	 (qualified_name_lookup_error): add a location_t parameter so that

The usual GCC style for ChangeLog entries is much less verbose -- just
the file name, function name, plus a description of the change.  Read
through the existing entries to get a feel for this; also the GNU
Coding Standards have a section describing it.

Dodji> diff --git a/gcc/cp/ChangeLog b/gcc/cp/ChangeLog
Dodji> index 636ee6a..140aea0 100644
[...]

The convention for gcc-patches is to send the ChangeLog entries
in plain format (just the stanza from the ChangeLog) and not as part
of the patch.  There are various ways to automate this; I usually use
an Emacs hack, but there are some scripts around too.

Dodji> +extern void qualified_name_lookup_error		(tree, tree, tree, location_t);

Lines over 80 columns should be wrapped.  I would insert a line break
before the 'location_t'.  This is true for declarations and
expressions, I saw a few instances in the patch.  There are some
(sketchy) guidelines in the standards for where to put line breaks.

Dodji> +  cp_token *token=NULL;

GCC uses spaces around most binary operators, like the '=' here.

Dodji> +      /*save the first token of the decl spec list for error reporting*/

Comments in GCC, by convention, should be full sentences which start
with a capital letter and end with a period followed by two spaces.
Also there is a space before the text; finally, there is a standard
for how to indent subsequent lines... so:

    /* Save the first token of the decl spec list for error
       reporting.  */

Dodji> +// Make sure column information is correctly shown in error reporting

I'm ambivalent about these additions.  The dg-options line explains it
pretty clearly.

Dodji> +  //following column number is not accurate enough but will make it for now
Dodji> +  static const ; // { dg-error "10: error: declaration does not declare anything" }

I don't want things like this to get lost.  It is too easy to forget
about them.

Dodji> diff --git a/gcc/testsuite/lib/g++.exp b/gcc/testsuite/lib/g++.exp
Dodji> -    lappend options "additional_flags=-fno-show-column"
Dodji> +#don't hardcode -fno-show-column
Dodji> +#lappend options "additional_flags=-fno-show-column"
Dodji>      lappend options "compiler=$GXX_UNDER_TEST"

Just delete this -- we generally don't comment out dead code in GCC.

thanks,
Tom