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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

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).



================
Comment at: include/clang/Basic/Attr.td:96
+                    [{!S->isStatic() && !S->isConst()}],
+                    "non-static non-const member functions">;
+
----------------
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!


================
Comment at: include/clang/Basic/Attr.td:2945
+def Reinitializes : InheritableAttr {
+  let Spellings = [CXX11<"clang", "reinitializes">];
+  let Subjects = SubjectList<[NonStaticNonConstCXXMethod], ErrorDiag>;
----------------
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.


================
Comment at: include/clang/Basic/AttrDocs.td:3426
+  let Content = [{
+The ``reinitializes`` attribute can be applied to a non-static, non-const C++
+member function to indicate that this member function reinitializes the entire
----------------
mboehme wrote:
> aaron.ballman wrote:
> > While I kind of understand the restriction on a `const` member function, what about code that has mutable members being reset within a const method?
> I find it hard to envision a method that reinitializes an object by modifying only mutable member variables. Mutable members are, after all, intended to be used for purposes (such as caching and locking) that do not change the "logical state" of the object, whereas reinitialization is an operation that does change the logical state of the object.
> 
> If it really does turn out that there are legitimate use cases for applying the 'reinitializes' attribute to a const member function, we can always relax this requirement later on.
That makes sense to me, thank you for the explanation.


Repository:
  rC Clang

https://reviews.llvm.org/D49911