ABataev added a comment. In D61509#1491979 <https://reviews.llvm.org/D61509#1491979>, @jdenny wrote:
> In D61509#1491898 <https://reviews.llvm.org/D61509#1491898>, @ABataev wrote: > > > Again, I don't see a single point why would we need this. I don't think > > there is a big difference between the location of pragma keyword and `omp` > > keyword. > > > @lebedev.ri : You said you're not sure changing the current start location > would matter for diagnostics, but do you believe we need to be concerned > about backward compatibility here for `Rewriter` users? For diagnostics, we don't use the location of `#pragma` or `omp` keywords often. If the diagnostic must be emitted for the whole pragma, it would be better to emit it for `#pragma` keyword, not `omp` keyword. I don't think there any rewriter users for OpenMP. Plus, if the problem exist (we point to `omp` keyword rather tan `#pragma`) better to fix this, if you can. > > >> Annotation tokens already have extra locations. Do you need the third one? > > Some pragmas already use that extra location. Roman suggested consistency > among all pragmas, and it would be nice if they stored the `#pragma` location > in a consistent manner. That was just a suggestion. YOu'd better to dicuss this with @rsmith. >> Plus, how you're going to store this location in the AST? We don't need any >> extra base classes. If you want to store this in AST nodes, add a new field >> to the existing classes and extended constructors/Create/(de-)serialization >> functions. > > If we're just adjusting OpenMP, I agree. However, Roman suggested > consistency among all pragmas, and I thought the base clase would make that > easier. Handling of other pragmas is completely different story. From OpenMP implementation point of view, we'll ned to store this extra location somewhere in the AST nodes for OpenMP. And you will need to do this for all the OpenMP pragmas (executable and declarative). If you're ging to store this location in AST nodes for OpenMP pragmas, I suggest not to add the new base class, but just extend the existing classes with the new data member for the location of the `#pragma` keyword. 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
