[PING] Use correct location information for OpenACC shape and simple clauses in C/C++

[Thomas Schwinge <thomas at codesourcery dot com>]

Hi!

On Wed, 27 Jul 2016 17:09:38 -0400, David Malcolm <dmalcolm@redhat.com> wrote:
> On Wed, 2016-07-27 at 17:17 +0200, Thomas Schwinge wrote:
> > I found that for a lot of OpenACC (and potentially also OpenMP)
> > clauses
> > (in C/C++ front ends; didn't look at Fortran), we use wrong location
> > information.  The problem is that
> > c_parser_oacc_all_clauses/c_parser_omp_all_clauses calls
> > cp_parser_omp_clause_name to determine the pragma_omp_clause c_kind,
> > and
> > that function (as documented) consumes the clause token before
> > returning.
> > So, when we then do "c_parser_peek_token (parser)->location" or
> > similar
> > in some clause parsing function, that will return the location
> > information of the token _after_ the clause token, which -- at least
> > very
> > often -- is not desirable, in particular if that location information
> > is
> > used then in a build_omp_clause call, which should point to the
> > clause
> > token itself, and not whatever follows after that.
> > 
> > Probably that all went unnoticed for so long, because the GCC
> > testsuite
> > largely is running with -fno-diagnostics-show-caret, so we don't
> > visually
> > see the wrong location information (and nobody pays attention to the
> > colum information as given, for example, as line 2, column 32 in
> > "[...]/c-c++-common/goacc/routine-2.c:2:32: error: [...]".
> 
> > There seems to be a lot of inconsistency in that in all the clause
> > parsing; here is just a first patch to fix the immediate problem I've
> > been observing.  OK for trunk already, or need to clean this all up
> > in
> > one go?  Do we need this on release branches, as a "quality of
> > implementation" fix (wrong diagnostic locations)?

> > [initial patch]

Ping for that one.


> I'm not a reviewer for the C/C++ FEs so I can't really review this
> patch

I think in your position as a maintainer for "diagnostic messages", you
should be qualified to exercise that status to approve such a patch.  :-)


> but it might be nice in this (or in a followup) to add some test
> cases for this that explicitly test the caret information for some of
> these errors.  [...]

> Hope this is constructive

It certainly is, thanks!  In fact, I had planned to look up how to do
that, which you've now made simpler by providing a specific receipe.


Grüße
 Thomas

Attachment: signature.asc
Description: PGP signature