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

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

mboehme marked 3 inline comments as done.
mboehme added a comment.

> Should this attribute have some semantic checking that ensures the non-static data members are accessed in the function that claims it reinitializes the object?

I think this would be hard to do in a way that provides meaningful value.

We can't demand that the function should modify all member variables because not all functions that perform a logical "clear" or other reinitialization need to do this. For example, a typical implementation of `std::string::clear()` only modifies the string length but leaves the capacity and buffer unchanged.

The strongest requirement I think we can impose is that the function needs to modify at least one of the member variables -- possibly indirectly (i.e. through a call to another function). I don't think checking this would provide a real benefit though. We already disallow the `reinitializes` attribute for const member functions -- so if someone misuses the attribute, it'll be on a non-const member function that most likely modifies member variables, just not in a way that's guaranteed to reinitialize the object.

What we'd really want to check is whether the function is actually doing a logical reinitialization of the object. Instead of putting the 'reinitializes' attribute on the function, we'd want some way of expressing "this function has the precondition `true` and the postcondition `empty()`", and then we'd like to prove this at compile time or at least check it at runtime. That's obviously beyond the scope of what is possible in C++ today though.



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


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


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


Repository:
  rC Clang

https://reviews.llvm.org/D49911