This is for use by clang-tidy's bugprone-use-after-move check -- see
corresponding clang-tidy patch at https://reviews.llvm.org/D49910.
Details
Diff Detail
- Repository
- rC Clang
- Build Status
Buildable 20859 Build 20859: arc lint + arc unit
Event Timeline
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? Otherwise, it seems like this would not trigger any diagnostics:
class C { int a, b; public: [[clang::reinitializes]] void clear() {} };
include/clang/Basic/Attr.td | ||
---|---|---|
96 | non-static, non-const member functions (with the comma) | |
2945 | 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? | |
include/clang/Basic/AttrDocs.td | ||
3426 | 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? | |
test/SemaCXX/attr-reinitializes.cpp | ||
4 | Please spell out the full diagnostic the first time it appears, and shorten the other uses, if you'd like. | |
10 | Please also add tests that demonstrate the attribute is intended to accept no args. |
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.
include/clang/Basic/Attr.td | ||
---|---|---|
96 | 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? | |
2945 | 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! | |
include/clang/Basic/AttrDocs.td | ||
3426 | 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. |
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).
include/clang/Basic/Attr.td | ||
---|---|---|
96 | 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! | |
2945 | 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. | |
include/clang/Basic/AttrDocs.td | ||
3426 | That makes sense to me, thank you for the explanation. |
non-static, non-const member functions (with the comma)