This is an archive of the discontinued LLVM Phabricator instance.

Summary:Add clang::reinitializes attribute
ClosedPublic

Authored by mboehme on Jul 27 2018, 5:30 AM.

Diff Detail

Event Timeline

mboehme created this revision.Jul 27 2018, 5:30 AM
mboehme retitled this revision from Summary: Add clang::reinitializes attribute to Summary:Add clang::reinitializes attribute.Jul 27 2018, 5:45 AM
mboehme added a reviewer: aaron.ballman.

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.

mboehme updated this revision to Diff 158051.Jul 30 2018, 12:53 PM

Various changes in response to reviewer comments.

mboehme marked 3 inline comments as done.Jul 30 2018, 1:10 PM

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.

aaron.ballman accepted this revision.Aug 2 2018, 1:07 PM

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.

This revision is now accepted and ready to land.Aug 2 2018, 1:07 PM
mboehme updated this revision to Diff 159007.Aug 3 2018, 7:29 AM
mboehme marked an inline comment as done.

Various changes in response to review comments.

mboehme marked 2 inline comments as done.Aug 3 2018, 7:32 AM

Thanks for the review!

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.

include/clang/Basic/Attr.td
96

No worries!

2945

Thanks for the explanation -- done!

I've also added a test that the GNU spelling is recognized.

mboehme updated this revision to Diff 160348.Aug 13 2018, 6:50 AM
mboehme marked 2 inline comments as done.

Rebase to head.

This revision was automatically updated to reflect the committed changes.