aaron.ballman added inline comments.
================ Comment at: clang/lib/Parse/ParseDeclCXX.cpp:4285 + // created for us by the caller. + return true; + } ---------------- aaron.ballman wrote: > erichkeane wrote: > > ABataev wrote: > > > erichkeane wrote: > > > > aaron.ballman wrote: > > > > > erichkeane wrote: > > > > > > Another place to make the same comment :D I wonder if giving a > > > > > > diagnostic on attempting to use omp::directive in a previous > > > > > > version should have either an extension warning or more explicit > > > > > > warning would be a good idea? > > > > > Ah, now I see what you're after. We could do that, but I think > > > > > deciding whether to expose this in older modes is more of an open > > > > > question. I went with the conservative approach first, but we could > > > > > relax this later. > > > > Well, I was going for 1 of two things: > > > > > > > > 1- allow in older modes, then do a warning that you're using it in the > > > > wrong version. > > > > > > > > 2- Customize the error from "unknown attribute directive" to something > > > > like, "attribute omp::directive is only available in OMP5.1 mode". > > > I think we can enable it for the previous versions, at least as an > > > extension. Thoughts? > > It'll mean a larger test-surface I'd think, but there _IS_ precedent both > > ways. > > IMO, we are probably better off doing #2 above (a better 'unknown > > attribute' diagnostic), then doing it as an extension if GCC makes that > > choice, or there is a compelling reaosn. > I think it makes sense to improve the diagnostic here, but I think we should > hold off on enabling it as an extension in older OpenMP modes until we have > some usage experience with the feature. Once we have confidence that the > feature works well in 5.1, then it's easy enough for us to support it in > older modes. Actually, I'm rethinking whether that makes sense. In order to have the new diagnostic, we'd have to perform this check *before* the `hasAttribute()` check above because the attributes aren't known. That adds a bit more checking back into the patch that was just removed (not the end of the world) and it makes out-of-spec OpenMP attributes behave differently than other out-of-spec kinds of attributes (which is strange). I think my preference is to leave this with the generic "unknown attribute" diagnostic. If we decide to enable it as an extension, then `hasAttribute()` will know about them and we can more easily add the forward/backward compat warnings at that point. WDYT? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105648/new/ https://reviews.llvm.org/D105648 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
