erichkeane added inline comments.
================ Comment at: clang/include/clang/Basic/TokenKinds.def:869 PRAGMA_ANNOTATION(pragma_openmp) +PRAGMA_ANNOTATION(pragma_openmp_from_attr) PRAGMA_ANNOTATION(pragma_openmp_end) ---------------- aaron.ballman wrote: > erichkeane wrote: > > `pragma_openmp_from_attr`? Funny since it isn't a pragma... > All of our pragma annotation tokens start with `pragma` (and we generate a > pragma annotation token to handle the parsing), so this is intended to convey > that it's a pragma_openmp annotation token that comes from_attr. > > But if you have a better name you'd prefer I use, I could certainly switch, > but I think we want to keep `pragma_openmp` as the prefix to make it clear > that this is paired with the preceding `pragma_openmp` annotation token. I guess I can buy that... it ends up being a little disorienting to see that... to me pragma_ meant #pragma, but I guess it could mean PRAGMA_ANNOTATION(. That said, I would think PRAGMA_ANNOTATION is represented by the annot the macro adds... I guess I would lean towards this being `openmp_attr`, but I don't feel strongly about that. ================ Comment at: clang/lib/Basic/Attributes.cpp:26 + // machinery that goes through TableGen. + if (LangOpts.OpenMP >= 51 && ScopeName == "omp") + return Name == "directive" || Name == "sequence"; ---------------- aaron.ballman wrote: > erichkeane wrote: > > Should we diagnose in non-OMP5.1 uses of this? > What would we diagnose here? This is `__has_cpp_attribute`'s internal > implementation, so I don't think we want a diagnostic here. I asked Mike Rice > whether he thought we should support this syntax in older OpenMP modes and > his recommendation was to not do that yet (I guess it's less common to > backport features to older OpenMP standard modes). Ah! I missed that this was that context. Disregard :) ================ Comment at: clang/lib/Parse/ParseDeclCXX.cpp:4285 + // created for us by the caller. + return true; + } ---------------- 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". Repository: rG LLVM Github Monorepo 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
