[PATCH] D61509: [PragmaHandler][OpenMP] Expose `#pragma` location

Fri May 3 08:58:32 PDT 2019

jdenny added a comment.

In D61509#1489771 <https://reviews.llvm.org/D61509#1489771>, @ABataev wrote:

> In D61509#1489761 <https://reviews.llvm.org/D61509#1489761>, @jdenny wrote:
>
> > In D61509#1489752 <https://reviews.llvm.org/D61509#1489752>, @ABataev wrote:
> >
> > > If the patch is going to be accepted, then definitely it must be solution #1.
> >
> >
> > I certainly have no objection to that and will be happy to implement it, but can you help me to understand the rationale?  (Thanks for your quick response!)
>
>
> Another one annotation token may significantly change the parsing process. It will require a lot of rework in the parsing of OpenMP pragmas plus may lead to some unpredictable results like endless parsing in some cases etc.

It seems simple to just consistently replace the one token with two consecutive tokens.  That much is easy enough for me to try out (without yet integrating the locations into the AST, which would take much longer).  Would you expect the test suite to catch the parsing problems you anticipate?

> It is much easier to change the tests rather than modify the whole parsing procedure.

I presented that argument poorly.  My main concern for solution #1 is that it might break backward compatibility for existing users of the AST.  However, I don't actually have evidence any user cares.  Do we generally offer backward compatibility guarantees here at all?  If this is not an important concern, then solution #1 seems obviously right to me too.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61509