On Thu, 2016-08-04 at 16:54 +0200, Thomas Schwinge wrote: > Hi! > > On Wed, 27 Jul 2016 17:09:38 -0400, David Malcolm < > dmalc...@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. > :-)
I don't know exactly where the boundaries of that role are; I've been assuming it means anything relating to the diagnostic subsystem itself (and location-tracking), as opposed to *usage* of the system. The patch in question is more about the latter. That said, your patch looks very reasonable to me (but as I mentioned, a test case demonstrating the improved caret locations would be good). Steering committee: am I being too conservative in my interpretation of that role? > > > 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
