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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
