[PATCH] D56113: [OpenMP] Replace predetermined shared for const variable

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D56113#1345337 <https://reviews.llvm.org/D56113#1345337>, @ABataev wrote:

> > In D56113#1345238 <https://reviews.llvm.org/D56113#1345238>, @ABataev wrote:
> > 
> >> In D56113#1345232 <https://reviews.llvm.org/D56113#1345232>, @jdenny wrote:
> >>
> >> > In D56113#1345047 <https://reviews.llvm.org/D56113#1345047>, @ABataev wrote:
> >> >
> >> > > In D56113#1344210 <https://reviews.llvm.org/D56113#1344210>, @jdenny wrote:
> >> > >
> >> > > > In D56113#1342879 <https://reviews.llvm.org/D56113#1342879>, @ABataev wrote:
> >> > > >
> >> > > > > But you will need another serie of patches for `reduction` and `linear` clauses to update their error messages.
> >> > > >
> >> > > >
> >> > > > Those already had their own checks for const.  I'm not aware of any cases not handled, and the test suite does pass after my patch.
> >> > >
> >> > >
> >> > > Yes, they have the checks for constness, but you need to  update those checks to emit this new error message for const values with mutable fields.
> >> >
> >> >
> >> > I believe you mean we should reuse `rejectConstNotMutableType` for `reduction` and `linear` rather than leave some of its implementation duplicated for those.
> >>
> > 
> > 
> > For reductions, this will change the message from "const-qualified list item cannot be reduction" to "const-qualified variable cannot be reduction".  Is that OK?  (There are many tests to update, so I want to ask first.)
>
> Mmmm. For the reductions better to keep the original message, because the list items also might be the array sections, not only the variables.


I need to add a parameter to `rejectConstNotMutableType` to specify whether decl/def notes are included, and I can reuse that parameter for this purpose.  Whether the diagnostic says "list item" or "variable" will then depend on what the list item is (variable, array section, etc.) rather than what the clause is (private, reduction, etc.).  I think that's ok.


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

https://reviews.llvm.org/D56113