jdenny added a comment. 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. That is, I believe we should //not// relax the existing checks for `reduction` and `linear` so that they permit const values with mutable fields. First, that doesn't seem possible for `linear`, which requires integral or pointer type. Second, that seems wrong for `reduction` because, for whatever reason, OpenMP 5.0 sec. 2.19.5.1 page 298 says: A list item that appears in a reduction clause must not be const-qualified. For that case, I can extend `rejectConstNotMutableType` with a bool parameter to indicate mutable fields are not permitted. Does all that make sense? > > >> By the way, is there any value to keeping the predetermined shared for const >> if -openmp-version=3.1 or earlier? > > Yes, you can check for the value of `LangOpts.OpenMP`. For OpenMP 3.1 it will > have the value `31`. How far back should we take this? I'm inclined to check for `30` and `31` only and assume anything else is newer, but let me know if we need to check for earlier versions. > > >>>> Sorry, I misunderstood your first response. When I try an atomic write to >>>> the shared variable, the difference between the uncombined and combined >>>> target teams directive affects functionality when targeting multicore. >>>> Does that matter? >>> >>> Yes, I'm aware if this problem. But I don't think it is very important and >>> can cause serious problems in user code. If you think that it is worth to >>> fix this prblem, you can go ahead and fix it. >> >> No, not worthwhile for me right now. I just wanted to point it out in >> passing in case it might matter to someone. Does a bugzilla seem worthwhile >> to you? > > Yes, generally speaking it would be good to fix this. So, yes, at least to > keep tracking, add a bug report to the bugzilla. Will do. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56113/new/ https://reviews.llvm.org/D56113 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
