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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
