This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Set pragma start loc to `#pragma` loc
ClosedPublic

Authored by jdenny on May 3 2019, 7:40 AM.

Details

Summary

This patch adjusts PragmaOpenMPHandler to set the location of
tok::annot_pragma_openmp to the #pragma location instead of the
omp location so that the former becomes the start location of the
OpenMP AST node. This can be useful when, for example, rewriting a
directive using Clang's Rewrite facility. Most of this patch updates
tests for changes to locations in diagnostics and -ast-dump output.

Diff Detail

Repository
rL LLVM

Event Timeline

jdenny created this revision.May 3 2019, 7:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2019, 7:40 AM

If the patch is going to be accepted, then definitely it must be solution #1.

jdenny added a comment.May 3 2019, 7:51 AM

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!)

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 is much easier to change the tests rather than modify the whole parsing procedure.

jdenny added a comment.May 3 2019, 8:30 AM

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.

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.

jdenny added a comment.May 3 2019, 9:09 AM

Thanks for explaining. I'll proceed with solution #1.

jdenny updated this revision to Diff 198046.May 3 2019, 10:57 AM
jdenny edited the summary of this revision. (Show Details)

As discussed, implement OpenMP solution #1 (one-line change in PragmaOpenMPHandler::HandlePragma in ParsePragma.cpp), and update tests.

The OpenMP part looks good.

lebedev.ri added a subscriber: lebedev.ri.

I recommend to split this into two parts - changing PragmaIntroducerKind Introducer to
struct NameMe { PragmaIntroducerKind Kind; SourceLocation Loc};, and the actual openmp change.
For that change, just basing off the clang-tidy diff, neither variant is ideal, so i'd like to see the proposed use-case.

clang/include/clang/Lex/Pragma.h
68–69 ↗(On Diff #198046)

PragmaIntroducerKind Introducer and SourceLocation IntroducerLoc are talking about the same thing, are they not?
They should be put into some new struct, instead of adding a new parameter.

jdenny added a comment.May 5 2019, 8:05 AM

I recommend to split this into two parts - changing PragmaIntroducerKind Introducer to
struct NameMe { PragmaIntroducerKind Kind; SourceLocation Loc};, and the actual openmp change.

Sure, I'll work on that. What about NameMe = PragmaIntroducer?

For that change, just basing off the clang-tidy diff, neither variant is ideal,

Do you mean that it's better for diagnostics that point to a pragma not to include the #pragma in their locations? If so, why is that?

The other variant I originally proposed doesn't alter existing locations and so wouldn't affect diagnostics, but Alexey pointed out potential issues for changes it requires in the parser.

so i'd like to see the proposed use-case.

That's a bit tricky. I'll explain.

My use case right now is not for OpenMP locations but for OpenACC locations in our Clacc project, which is not upstream. Clacc is calling Rewriter and passing locations from the OpenACC AST in order to rewrite the OpenACC pragmas to OpenMP. Without the location for an OpenACC pragma's #pragma, Clacc cannot easily remove an OpenACC pragma or insert code before it. At least not that I found. Of course, I'd be happy for someone to explain to me how Rewriter was designed to be used differently. I'd also be happy to share relevant code samples from Clacc if that would help.

So what does this have to do with OpenMP locations? I'm anticipating that someone will one day want to use Rewriter in this way with other pragmas, so I'm offering some scaffolding toward that goal for all pragmas, and I'm completing the fix only for OpenMP, which is the existing pragma set I understand best. Moreover, offering this now facilitates keeping Clacc up to date with master and eventually considering it for upstreaming.

Thanks for your responses.

I recommend to split this into two parts - changing PragmaIntroducerKind Introducer to
struct NameMe { PragmaIntroducerKind Kind; SourceLocation Loc};, and the actual openmp change.

Sure, I'll work on that. What about NameMe = PragmaIntroducer?

Could work. And then move PragmaIntroducerKind into it.
I believe this part of the refactoring is completely uncontroversial.

For that change, just basing off the clang-tidy diff, neither variant is ideal,

Do you mean that it's better for diagnostics that point to a pragma not to include the #pragma in their locations? If so, why is that?

I'm not sure either one is better than the other one.

I have two concerns:

  • I fear this would result in inconsistency with other pragmas, since this will only change openmp-ones. I don't know if it will be accepted to migrate the rest of them in the same way.
  • This use-case requires having the location of #pragma, so the entire AST is migrating to store it. But the current location will no longer be accessible from AST.

I see two paths forward:

  • Mail cfe-dev, and suggest to do this change for *all* pragmas. Either this is ok for all of them, or none of them.
  • Moar abstractions - how about not changing the startloc of openmp directives, but instead add some baseclass to OMPDirective class (& every other class that is created from pragma), that would record the PragmaIntroducer?

The other variant I originally proposed doesn't alter existing locations and so wouldn't affect diagnostics, but Alexey pointed out potential issues for changes it requires in the parser.

so i'd like to see the proposed use-case.

That's a bit tricky. I'll explain.

My use case right now is not for OpenMP locations but for OpenACC locations in our Clacc project, which is not upstream. Clacc is calling Rewriter and passing locations from the OpenACC AST in order to rewrite the OpenACC pragmas to OpenMP. Without the location for an OpenACC pragma's #pragma, Clacc cannot easily remove an OpenACC pragma or insert code before it. At least not that I found. Of course, I'd be happy for someone to explain to me how Rewriter was designed to be used differently. I'd also be happy to share relevant code samples from Clacc if that would help.

So what does this have to do with OpenMP locations? I'm anticipating that someone will one day want to use Rewriter in this way with other pragmas, so I'm offering some scaffolding toward that goal for all pragmas, and I'm completing the fix only for OpenMP, which is the existing pragma set I understand best. Moreover, offering this now facilitates keeping Clacc up to date with master and eventually considering it for upstreaming.

Thanks for your responses.

ABataev added a comment.EditedMay 5 2019, 9:19 AM

I recommend to split this into two parts - changing PragmaIntroducerKind Introducer to
struct NameMe { PragmaIntroducerKind Kind; SourceLocation Loc};, and the actual openmp change.

Sure, I'll work on that. What about NameMe = PragmaIntroducer?

Could work. And then move PragmaIntroducerKind into it.
I believe this part of the refactoring is completely uncontroversial.

For that change, just basing off the clang-tidy diff, neither variant is ideal,

Do you mean that it's better for diagnostics that point to a pragma not to include the #pragma in their locations? If so, why is that?

I'm not sure either one is better than the other one.

I have two concerns:

  • I fear this would result in inconsistency with other pragmas, since this will only change openmp-ones. I don't know if it will be accepted to migrate the rest of them in the same way.
  • This use-case requires having the location of #pragma, so the entire AST is migrating to store it. But the current location will no longer be accessible from AST.

I see two paths forward:

  • Mail cfe-dev, and suggest to do this change for *all* pragmas. Either this is ok for all of them, or none of them.
  • Moar abstractions - how about not changing the startloc of openmp directives, but instead add some baseclass to OMPDirective class (& every other class that is created from pragma), that would record the PragmaIntroducer?

I'm against this solution. I don't see any reasons why we should do this. Instead, we're getting a lot of pain with parsing and maintenance.

The other variant I originally proposed doesn't alter existing locations and so wouldn't affect diagnostics, but Alexey pointed out potential issues for changes it requires in the parser.

so i'd like to see the proposed use-case.

That's a bit tricky. I'll explain.

My use case right now is not for OpenMP locations but for OpenACC locations in our Clacc project, which is not upstream. Clacc is calling Rewriter and passing locations from the OpenACC AST in order to rewrite the OpenACC pragmas to OpenMP. Without the location for an OpenACC pragma's #pragma, Clacc cannot easily remove an OpenACC pragma or insert code before it. At least not that I found. Of course, I'd be happy for someone to explain to me how Rewriter was designed to be used differently. I'd also be happy to share relevant code samples from Clacc if that would help.

So what does this have to do with OpenMP locations? I'm anticipating that someone will one day want to use Rewriter in this way with other pragmas, so I'm offering some scaffolding toward that goal for all pragmas, and I'm completing the fix only for OpenMP, which is the existing pragma set I understand best. Moreover, offering this now facilitates keeping Clacc up to date with master and eventually considering it for upstreaming.

Thanks for your responses.

jdenny added a comment.May 5 2019, 4:52 PM

I recommend to split this into two parts - changing PragmaIntroducerKind Introducer to
struct NameMe { PragmaIntroducerKind Kind; SourceLocation Loc};, and the actual openmp change.

Sure, I'll work on that. What about NameMe = PragmaIntroducer?

Could work. And then move PragmaIntroducerKind into it.
I believe this part of the refactoring is completely uncontroversial.

For that change, just basing off the clang-tidy diff, neither variant is ideal,

Do you mean that it's better for diagnostics that point to a pragma not to include the #pragma in their locations? If so, why is that?

I'm not sure either one is better than the other one.

I have two concerns:

  • I fear this would result in inconsistency with other pragmas, since this will only change openmp-ones. I don't know if it will be accepted to migrate the rest of them in the same way.
  • This use-case requires having the location of #pragma, so the entire AST is migrating to store it. But the current location will no longer be accessible from AST.

I see two paths forward:

  • Mail cfe-dev, and suggest to do this change for *all* pragmas. Either this is ok for all of them, or none of them.
  • Moar abstractions - how about not changing the startloc of openmp directives,

My alternative proposal was exactly that. A difficulty is how to pass the #pragma location to the OpenMP AST node constructors. PragmaOpenMPHandler::HandlePragma passes locations via the tok::annot_pragma_openmp and tok::annot_pragma_openmp_end tokens, so where do we pass this new location? I proposed creating a third token, and Alexey was concerned over the parsing problems that would create.

but instead add some baseclass to `OMPDirective` class (& every other class that is created from pragma), that would record the `PragmaIntroducer`?

I agree that a base class would be a nice way to extend all pragma classes with the PragmaIntroducer.

I'm against this solution. I don't see any reasons why we should do this. Instead, we're getting a lot of pain with parsing and maintenance.

One way to avoid creating an extra token would be to widen the Token class to store the additional location. The Token documentation says it's not intended to be space efficient. How does that sound to people?

I recommend to split this into two parts - changing PragmaIntroducerKind Introducer to
struct NameMe { PragmaIntroducerKind Kind; SourceLocation Loc};, and the actual openmp change.

Sure, I'll work on that. What about NameMe = PragmaIntroducer?

Could work. And then move PragmaIntroducerKind into it.
I believe this part of the refactoring is completely uncontroversial.

For that change, just basing off the clang-tidy diff, neither variant is ideal,

Do you mean that it's better for diagnostics that point to a pragma not to include the #pragma in their locations? If so, why is that?

I'm not sure either one is better than the other one.

I have two concerns:

  • I fear this would result in inconsistency with other pragmas, since this will only change openmp-ones. I don't know if it will be accepted to migrate the rest of them in the same way.
  • This use-case requires having the location of #pragma, so the entire AST is migrating to store it. But the current location will no longer be accessible from AST.

I see two paths forward:

  • Mail cfe-dev, and suggest to do this change for *all* pragmas. Either this is ok for all of them, or none of them.
  • Moar abstractions - how about not changing the startloc of openmp directives,

My alternative proposal was exactly that. A difficulty is how to pass the #pragma location to the OpenMP AST node constructors. PragmaOpenMPHandler::HandlePragma passes locations via the tok::annot_pragma_openmp and tok::annot_pragma_openmp_end tokens, so where do we pass this new location? I proposed creating a third token, and Alexey was concerned over the parsing problems that would create.

but instead add some baseclass to `OMPDirective` class (& every other class that is created from pragma), that would record the `PragmaIntroducer`?

I agree that a base class would be a nice way to extend all pragma classes with the PragmaIntroducer.

I'm against this solution. I don't see any reasons why we should do this. Instead, we're getting a lot of pain with parsing and maintenance.

One way to avoid creating an extra token would be to widen the Token class to store the additional location. The Token documentation says it's not intended to be space efficient. How does that sound to people?

That was my proposal, yes.

clang-tools-extra/test/clang-tidy/openmp-exception-escape.cpp
1 ↗(On Diff #198046)

Changes to this file will no longer be needed, i have changed the check.

jdenny added a comment.May 6 2019, 6:51 AM

I recommend to split this into two parts - changing PragmaIntroducerKind Introducer to
struct NameMe { PragmaIntroducerKind Kind; SourceLocation Loc};, and the actual openmp change.

Sure, I'll work on that. What about NameMe = PragmaIntroducer?

Could work. And then move PragmaIntroducerKind into it.
I believe this part of the refactoring is completely uncontroversial.

For that change, just basing off the clang-tidy diff, neither variant is ideal,

Do you mean that it's better for diagnostics that point to a pragma not to include the #pragma in their locations? If so, why is that?

I'm not sure either one is better than the other one.

I have two concerns:

  • I fear this would result in inconsistency with other pragmas, since this will only change openmp-ones. I don't know if it will be accepted to migrate the rest of them in the same way.
  • This use-case requires having the location of #pragma, so the entire AST is migrating to store it. But the current location will no longer be accessible from AST.

I see two paths forward:

  • Mail cfe-dev, and suggest to do this change for *all* pragmas. Either this is ok for all of them, or none of them.
  • Moar abstractions - how about not changing the startloc of openmp directives,

My alternative proposal was exactly that. A difficulty is how to pass the #pragma location to the OpenMP AST node constructors. PragmaOpenMPHandler::HandlePragma passes locations via the tok::annot_pragma_openmp and tok::annot_pragma_openmp_end tokens, so where do we pass this new location? I proposed creating a third token, and Alexey was concerned over the parsing problems that would create.

but instead add some baseclass to `OMPDirective` class (& every other class that is created from pragma), that would record the `PragmaIntroducer`?

I agree that a base class would be a nice way to extend all pragma classes with the PragmaIntroducer.

I'm against this solution. I don't see any reasons why we should do this. Instead, we're getting a lot of pain with parsing and maintenance.

One way to avoid creating an extra token would be to widen the Token class to store the additional location. The Token documentation says it's not intended to be space efficient. How does that sound to people?

That was my proposal, yes.

@ABataev : Does that address your concerns?

I recommend to split this into two parts - changing PragmaIntroducerKind Introducer to
struct NameMe { PragmaIntroducerKind Kind; SourceLocation Loc};, and the actual openmp change.

Sure, I'll work on that. What about NameMe = PragmaIntroducer?

Could work. And then move PragmaIntroducerKind into it.
I believe this part of the refactoring is completely uncontroversial.

For that change, just basing off the clang-tidy diff, neither variant is ideal,

Do you mean that it's better for diagnostics that point to a pragma not to include the #pragma in their locations? If so, why is that?

I'm not sure either one is better than the other one.

I have two concerns:

  • I fear this would result in inconsistency with other pragmas, since this will only change openmp-ones. I don't know if it will be accepted to migrate the rest of them in the same way.
  • This use-case requires having the location of #pragma, so the entire AST is migrating to store it. But the current location will no longer be accessible from AST.

I see two paths forward:

  • Mail cfe-dev, and suggest to do this change for *all* pragmas. Either this is ok for all of them, or none of them.
  • Moar abstractions - how about not changing the startloc of openmp directives,

My alternative proposal was exactly that. A difficulty is how to pass the #pragma location to the OpenMP AST node constructors. PragmaOpenMPHandler::HandlePragma passes locations via the tok::annot_pragma_openmp and tok::annot_pragma_openmp_end tokens, so where do we pass this new location? I proposed creating a third token, and Alexey was concerned over the parsing problems that would create.

but instead add some baseclass to `OMPDirective` class (& every other class that is created from pragma), that would record the `PragmaIntroducer`?

I agree that a base class would be a nice way to extend all pragma classes with the PragmaIntroducer.

I'm against this solution. I don't see any reasons why we should do this. Instead, we're getting a lot of pain with parsing and maintenance.

One way to avoid creating an extra token would be to widen the Token class to store the additional location. The Token documentation says it's not intended to be space efficient. How does that sound to people?

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.

I recommend to split this into two parts - changing PragmaIntroducerKind Introducer to
struct NameMe { PragmaIntroducerKind Kind; SourceLocation Loc};, and the actual openmp change.

Sure, I'll work on that. What about NameMe = PragmaIntroducer?

Could work. And then move PragmaIntroducerKind into it.
I believe this part of the refactoring is completely uncontroversial.

For that change, just basing off the clang-tidy diff, neither variant is ideal,

Do you mean that it's better for diagnostics that point to a pragma not to include the #pragma in their locations? If so, why is that?

I'm not sure either one is better than the other one.

I have two concerns:

  • I fear this would result in inconsistency with other pragmas, since this will only change openmp-ones. I don't know if it will be accepted to migrate the rest of them in the same way.
  • This use-case requires having the location of #pragma, so the entire AST is migrating to store it. But the current location will no longer be accessible from AST.

I see two paths forward:

  • Mail cfe-dev, and suggest to do this change for *all* pragmas. Either this is ok for all of them, or none of them.
  • Moar abstractions - how about not changing the startloc of openmp directives,

My alternative proposal was exactly that. A difficulty is how to pass the #pragma location to the OpenMP AST node constructors. PragmaOpenMPHandler::HandlePragma passes locations via the tok::annot_pragma_openmp and tok::annot_pragma_openmp_end tokens, so where do we pass this new location? I proposed creating a third token, and Alexey was concerned over the parsing problems that would create.

but instead add some baseclass to `OMPDirective` class (& every other class that is created from pragma), that would record the `PragmaIntroducer`?

I agree that a base class would be a nice way to extend all pragma classes with the PragmaIntroducer.

I'm against this solution. I don't see any reasons why we should do this. Instead, we're getting a lot of pain with parsing and maintenance.

One way to avoid creating an extra token would be to widen the Token class to store the additional location. The Token documentation says it's not intended to be space efficient. How does that sound to people?

That was my proposal, yes.

@ABataev : Does that address your concerns?

Annotation tokens already have extra locations. Do you need the third one? 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.

jdenny added a comment.May 6 2019, 7:56 AM

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?

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.

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.

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.

Thanks for everyone's responses so far.

At this point, I'm going to follow the uncontroversial suggestions from @lebedev.ri: create a struct PragmaIntroducer, and split the OpenMP work into a second patch. That will hopefully make it easier for @rsmith or others to consider what we have now and what should be done for other pragmas now, if anything.

jdenny updated this revision to Diff 198488.May 7 2019, 9:03 AM
jdenny retitled this revision from [PragmaHandler][OpenMP] Expose `#pragma` location to [PragmaHandler] Expose `#pragma` location.
jdenny edited the summary of this revision. (Show Details)
jdenny set the repository for this revision to rG LLVM Github Monorepo.

As suggested, I've created a struct PragmaIntroducer and I've reduced this patch not to include the OpenMP changes. I'll add a new patch with the OpenMP changes soon.

There are a few additional changes here reviewers might want to check out:

  • This time I noticed that clang/docs/ClangPlugins.rst and clang/examples/AnnotateFunctions need to be updated for the change to PragmaHandler parameters, so I did that. Does that represent an important break in backward compatibility?
  • I've also used PragmaIntroducer to simplify clang::Preprocessor::HandlePragmaDirective, which already accepted a SourceLocation and PragmaIntroducerKind before any of my patches.
  • I didn't do the same for clang::PPCallbacks::PragmaDirective because that changes the API documented in clang-tools-extra/docs/pp-trace.rst, and I'm not sure about backward compatibility guarantees there.

It would have been better to submit this refactor as a new patch..

jdenny marked an inline comment as done.May 7 2019, 9:10 AM
jdenny added a comment.May 7 2019, 9:15 AM

It would have been better to submit this refactor as a new patch..

Sorry, I didn't realize that was the norm. I can do that now if it would help. I can also revert changes to this patch if the goal is to make it easier to reference the old version.

It would have been better to submit this refactor as a new patch..

Sorry, I didn't realize that was the norm. I can do that now if it would help. I can also revert changes to this patch if the goal is to make it easier to reference the old version.

I think that would be good. This current diff would be a simple NFC cleanup, i think i will signoff it even.

jdenny added a comment.May 7 2019, 9:22 AM

It would have been better to submit this refactor as a new patch..

Sorry, I didn't realize that was the norm. I can do that now if it would help. I can also revert changes to this patch if the goal is to make it easier to reference the old version.

I think that would be good. This current diff would be a simple NFC cleanup, i think i will signoff it even.

Thanks, I'll do that.

For future reference, I assume abandoning and starting a new review is better here because I'm splitting the patch. Is there a more general rule of thumb on this?

I don't see why this differential can't be updated to only contain the remaining part
of the diff (for the actual OpenMP change), after splitting the NFC refactoring part.

jdenny added a comment.May 7 2019, 9:28 AM

I don't see why this differential can't be updated to only contain the remaining part
of the diff (for the actual OpenMP change), after splitting the NFC refactoring part.

OK

jdenny updated this revision to Diff 198491.May 7 2019, 9:46 AM
jdenny retitled this revision from [PragmaHandler] Expose `#pragma` location to [OpenMP] Set pragma start loc to `#pragma` loc.
jdenny edited the summary of this revision. (Show Details)

As requested, replace this patch with just the OpenMP changes.

Now that 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.
  1. Should we adjust all non-OpenMP pragmas in the same way immediately, or should we adjust them gradually as the need arises?

If the answers to the above questions are "replace" and "gradually", then I believe this patch is ready. (It's just a tweak in clang/lib/Parse/ParsePragma.cpp and a ton of test updates.)

By the way, in D61643, @Meinersbur pointed out that @rsmith also wanted to see diagnostics point at a #pragma: https://bugs.llvm.org/show_bug.cgi?id=41514#c1

If @rsmith and others don't get a chance to chime in, then I suppose an RFC should be next.

  1. Is there a diagnostic that would point to the omp token? As much as I like complete info (such as SourceLoc of semicolons), I cannot think of a use case for it.
  2. I would like to see all of them all adjusted. There is an immediate improvement in that improves the diagnostic output.
  1. Is there a diagnostic that would point to the omp token? As much as I like complete info (such as SourceLoc of semicolons), I cannot think of a use case for it.
  2. I would like to see all of them all adjusted. There is an immediate improvement in that improves the diagnostic output.
  1. Nope, there is no omp specific diagnostics
  1. Is there a diagnostic that would point to the omp token? As much as I like complete info (such as SourceLoc of semicolons), I cannot think of a use case for it.
  2. I would like to see all of them all adjusted. There is an immediate improvement in that improves the diagnostic output.
  1. Nope, there is no omp specific diagnostics
  1. Based on the test suite changes this patch makes, I agree for OpenMP. I haven't explored other pragmas.
  1. I too think it likely makes sense to adjust them all eventually. But do people think it's important to write patches adjusting all pragmas before pushing the adjustment for any of them?
  1. I too think it likely makes sense to adjust them all eventually. But do people think it's important to write patches adjusting all pragmas before pushing the adjustment for any of them?

I am not sure I understand. Do you mean whether you need all patches for each pragma to be accepted before you can commit the first? This is not that case. IMHO you can even put all of it into a single patch as it should be very straightforward. The most work is adapting the tests.

  1. I too think it likely makes sense to adjust them all eventually. But do people think it's important to write patches adjusting all pragmas before pushing the adjustment for any of them?

I am not sure I understand. Do you mean whether you need all patches for each pragma to be accepted before you can commit the first? This is not that case.

@lebedev.ri expressed concern that it might not be acceptable to migrate all pragmas in the same way. That would suggest we must handle them all before committing any.

IMHO you can even put all of it into a single patch as it should be very straightforward. The most work is adapting the tests.

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.

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

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

If the answers to the above questions are "replace" and "gradually", then I believe this patch is ready. (It's just a tweak in clang/lib/Parse/ParsePragma.cpp and a ton of test updates.)

By the way, in D61643, @Meinersbur pointed out that @rsmith also wanted to see diagnostics point at a #pragma: https://bugs.llvm.org/show_bug.cgi?id=41514#c1

If @rsmith and others don't get a chance to chime in, then I suppose an RFC should be next.

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

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

aaron.ballman accepted this revision.May 22 2019, 11:58 AM

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.

These changes LGTM! I'd say give it a few days in case other reviewers would like to chime in.

This revision is now accepted and ready to land.May 22 2019, 11:58 AM

These changes LGTM!

Thanks, and thanks to everyone who chimed in.

I'd say give it a few days in case other reviewers would like to chime in.

Sure. Maybe this weekend or so.

  1. I too think it likely makes sense to adjust them all eventually. But do people think it's important to write patches adjusting all pragmas before pushing the adjustment for any of them?

I am not sure I understand. Do you mean whether you need all patches for each pragma to be accepted before you can commit the first? This is not that case.

@lebedev.ri expressed concern that it might not be acceptable to migrate all pragmas in the same way. That would suggest we must handle them all before committing any.

IMHO you can even put all of it into a single patch as it should be very straightforward. The most work is adapting the tests.

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

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.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 28 2019, 12:27 PM