[PATCH] D61509: [OpenMP] Set pragma start loc to `#pragma` loc

Wed May 22 11:18:25 PDT 2019

aaron.ballman added a comment.

In D61509#1512090 <https://reviews.llvm.org/D61509#1512090>, @jdenny wrote:

> Now that D61643 <https://reviews.llvm.org/D61643> is pushed, we're back to this patch.  My recollection is there are two remaining issues:
>
> 1. Should we store both the `#pragma` location and the `omp` location in the AST, or is it fine to just replace the latter location with the former?  If we choose to store both, we still haven't settled on an implementation, which likely will involve non-trivial changes in the parser and the AST.  For OpenMP, @ABataev said replacing should be fine.

Storing both would be nice, but not required, IMO. Storing the location of the pragma "namespace" would be useful for third-party tooling purposes, but I don't think we have a need for it in the frontend otherwise.

> 2. Should we adjust all non-OpenMP pragmas in the same way immediately, or should we adjust them gradually as the need arises?

We usually do incremental improvement unless that's impossible or would leave things in a badly inconsistent state. I don't think we need to adjust everything immediately in this case, but we should strive for quickly reaching eventual consistency.

> If the answers to the above questions are "replace" and "gradually", then I believe this patch is ready.  (It's just a tweak in `clang/lib/Parse/ParsePragma.cpp` and a ton of test updates.)
> 
> By the way, in D61643 <https://reviews.llvm.org/D61643>, @Meinersbur pointed out that @rsmith also wanted to see diagnostics point at a `#pragma`:  https://bugs.llvm.org/show_bug.cgi?id=41514#c1
> 
> If @rsmith and others don't get a chance to chime in, then I suppose an RFC should be next.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61509/new/

https://reviews.llvm.org/D61509