erichkeane added inline comments.
================ Comment at: clang/lib/Parse/ParseDeclCXX.cpp:4285 + // created for us by the caller. + return true; + } ---------------- aaron.ballman wrote: > 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? I guess my main motivation is that OMP version is a little bit more of a subtle difference, so I'd think the hint would be appreciated. That said, it DOES look like it is going to be enough of a hassle that we could perhaps do it as a future patch. There is perhaps an improvement todiagnostics in general here (for the Attribute owner, not the author of this patch :-P) to differentiate, "this attribute name is nonsense" vs "this attribute name is only available in higher language settings". 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
