jdenny added a comment. In D61509#1512601 <https://reviews.llvm.org/D61509#1512601>, @Meinersbur wrote:
> > I would think different people would want to review different pragmas, so > > separate patches would be better, but I'm happy to be corrected as I > > haven't explored who owns what here. > > AFICS it is changing ` Tok.setLocation(FirstTok.getLocation());` to ` > Tok.setLocation(Introducer.Loc);` for most PragmaHandlers that emit an > annotation token. I think the concern was more about the effect of the replacement than how to implement it. For example, the test changes in this patch provide evidence the effect on OpenMP seems fine. There might also be external use cases, so reviewers who are more familiar with specific pragmas (like Alexey for OpenMP) might have more info on those. > To be on the safe side, you can create a patch for each PragmaHandler > individually (otherwise a review may request to spit them up). You can commit > each accepted patch immediately unless a reviewer mentions that you should > wait for $event to happen, like @aaron.ballman just did. Sure. I discussed the possibility of adjusting all pragmas (to see the effect) before pushing any changes for exactly that reason: a reviewer previously suggested waiting due to his concerns over creating inconsistencies among pragmas, some of which might have use cases OpenMP doesn't, if we pushed the OpenMP changes in this patch immediately. I think the majority now feel it's fine to go ahead, but hopefully that isn't just because I didn't explain the previous concerns clearly. Anyway, I'll wait a few days to see if there are more comments. 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
