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

Joel E. Denny via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed May 22 11:39:31 PDT 2019


jdenny added a comment.

In D61509#1512488 <https://reviews.llvm.org/D61509#1512488>, @aaron.ballman wrote:

> 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.


Makes sense.  It sounds like we should go with the `#pragma` location for now and add the namespace location later if someone expresses a specific need.

> 
> 
>> 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.

The overall vote seems to be that this patch is ready to push, and we/I should work on other pragmas soon after.  I'm also happy to wait for more comments if people want more time.


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

https://reviews.llvm.org/D61509





More information about the cfe-commits mailing list