[PATCH] D49911: Summary:Add clang::reinitializes attribute

[Martin Böhme via Phabricator via cfe-commits]

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