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

Fri May 3 09:03:33 PDT 2019

ABataev added a comment.

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

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

I rather doubt there was many users. Plus, this change will change just the source location, nothing else. So I still suggest to use approach #1.
About test suite, we have some tests to try to catch the endless parsing case (it was reported already earlier and eas fixed), but the testing is not full, of course.
Also, the task of replacing one anotation token by 2 otehrs is not so straightforward, it might require some additional code changes and logic changes.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61509