mboehme marked 2 inline comments as done. mboehme added a comment. Thanks for the review!
In https://reviews.llvm.org/D49911#1186185, @aaron.ballman wrote: > The attribute itself is looking reasonable aside from some minor nits, but > this should not be committed until the clang-tidy functionality is approved > (since it has no utility beyond clang-tidy). Will do. ================ Comment at: include/clang/Basic/Attr.td:96 + [{!S->isStatic() && !S->isConst()}], + "non-static non-const member functions">; + ---------------- aaron.ballman wrote: > mboehme wrote: > > aaron.ballman wrote: > > > `non-static, non-const member functions` (with the comma) > > IIUC, Clang then translates the comma into an "and" -- so the actual > > diagnostic becomes "non-static and non-const member functions" (see the > > expected-error in the tests). Is this as intended? > Ugh, that's a deficiency in ClangAttrEmitter.cpp. Good thing diagnostics > aren't grammatically correct anyway, you can roll back to the previous form. > Sorry about the churn! No worries! ================ Comment at: include/clang/Basic/Attr.td:2945 +def Reinitializes : InheritableAttr { + let Spellings = [CXX11<"clang", "reinitializes">]; + let Subjects = SubjectList<[NonStaticNonConstCXXMethod], ErrorDiag>; ---------------- aaron.ballman wrote: > mboehme wrote: > > aaron.ballman wrote: > > > I think it makes sense to use `CXX11` instead of `Clang` as this > > > attribute doesn't make sense in C2x mode, but should there be a `GNU` > > > spelling as well? > > I have to admit I'm not sure. What are the usual considerations here? Does > > this need to be coordinated in any way with GNU, or can Clang simply > > introduce additional "GNU-style" spellings (i.e. with `__attribute__`) that > > are Clang-only? > > > > I understand there's a difference between `GCC` spellings and `GNU` > > spellings -- but I'm not sure what the rules around the latter are. Let me > > know if you think this should have a `GNU` spelling, and I'll add it! > We generally want things with both spellings unless there's rationale > justifying why only one spelling is better. I don't see any reason why this > shouldn't have a GNU spelling as well. > > I'd spell this as `Clang<"reinitializes", 0>` so it gets both the CXX11 and > GNU spellings but not the C2x spelling. Thanks for the explanation -- done! I've also added a test that the GNU spelling is recognized. Repository: rC Clang https://reviews.llvm.org/D49911 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
