This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Recognize [[clang::reinitializes]] attribute in bugprone-use-after-move
ClosedPublic

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

Details

Summary

This allows member functions to be marked as reinitializing the object. After a
moved-from object has been reinitialized, the check will no longer consider it
to be in an indeterminate state.

The patch that adds the attribute itself is at https://reviews.llvm.org/D49911

Diff Detail

Repository
rL LLVM

Event Timeline

mboehme created this revision.Jul 27 2018, 5:16 AM
mboehme updated this revision to Diff 157669.Jul 27 2018, 5:19 AM

[clang-tidy] Recognize [[clang::reinitializes]] attribute in bugprone-use-after-move.

This allows member functions to be marked as reinitializing the object. After a
moved-from object has been reinitialized, the check will no longer consider it
to be in an indeterminate state.

I'll submit a patch for Clang in a moment that adds the attribute itself.

Reviewers:
aaron.ballman, ilya-biryukov

Subscribers:
cfe-commits

mboehme retitled this revision from Reviewers: to [clang-tidy] Recognize [[clang::reinitializes]] attribute in bugprone-use-after-move.Jul 27 2018, 5:24 AM
mboehme edited the summary of this revision. (Show Details)
lebedev.ri added a project: Restricted Project.Jul 27 2018, 5:30 AM
mboehme edited the summary of this revision. (Show Details)Jul 27 2018, 5:31 AM

Any comments?

https://reviews.llvm.org/D49911, on which this relies, is now ready to land.

Are you going to propose adding this attribute to libc++, or is this expected to only work with UDTs?

Are you going to propose adding this attribute to libc++, or is this expected to only work with UDTs?

I don't have any experience contributing to libc++, but I think this would make sense.

The check currently hard-codes various member functions of classes in the "std" namespace that do reinitializations; I'm not sure though if those can be removed after the attribute has been added to libc++. We'd would also presumably have to add the attribute to libstdc++ -- does it accept Clang-only attributes? And what is the story for people using clang-tidy with MSVC projects? (I have to admit I'm very hazy on how that works...)

Are you going to propose adding this attribute to libc++, or is this expected to only work with UDTs?

I don't have any experience contributing to libc++, but I think this would make sense.

The check currently hard-codes various member functions of classes in the "std" namespace that do reinitializations; I'm not sure though if those can be removed after the attribute has been added to libc++. We'd would also presumably have to add the attribute to libstdc++ -- does it accept Clang-only attributes? And what is the story for people using clang-tidy with MSVC projects? (I have to admit I'm very hazy on how that works...)

I ask the question because it's novel to add an attribute to Clang that's used only by clang-tidy, and I'm wondering who is expected to use that attribute to help with this check. Is it expected to be an attribute users use with their own custom datatypes and it's not needed for standards-based APIs (because we handle those some other way), or something else? As it stands, I sort of feel like this is a very heavy approach to solve an edge case -- is there a lot of evidence for re-using a moved-from object after reinitializing it?

Are you going to propose adding this attribute to libc++, or is this expected to only work with UDTs?

I don't have any experience contributing to libc++, but I think this would make sense.

The check currently hard-codes various member functions of classes in the "std" namespace that do reinitializations; I'm not sure though if those can be removed after the attribute has been added to libc++. We'd would also presumably have to add the attribute to libstdc++ -- does it accept Clang-only attributes? And what is the story for people using clang-tidy with MSVC projects? (I have to admit I'm very hazy on how that works...)

I ask the question because it's novel to add an attribute to Clang that's used only by clang-tidy, and I'm wondering who is expected to use that attribute to help with this check. Is it expected to be an attribute users use with their own custom datatypes and it's not needed for standards-based APIs (because we handle those some other way), or something else? As it stands, I sort of feel like this is a very heavy approach to solve an edge case -- is there a lot of evidence for re-using a moved-from object after reinitializing it?

Ah, now I understand what you're getting at.

Yes, I see this attribute mainly as something that people would add to their own user-defined types -- typically, containers and smart pointers.

I've regularly been getting internal feature requests for this. One typical pattern in which this comes up is the following:

MyContainer<T> container;
T t;
while (GetNextT(&t)) {

container.Add(t);
if (SomeCondition()) {
  PassToConsumer(std::move(container));
  container.Clear();
}

}

I.e. you're incrementally adding items to some data structure, and at some point you decide the data structure is now complete, so you hand it off to a consumer and clear it, ready to be filled with the next batch of items.

As clang-tidy doesn't understand that the "container.Clear()" reinitializes the container, it complains that the Clear() is a use-after-move.

Yes, it's unusual to add an attribute to Clang that is only (at least initially) intended for use by clang-tidy -- but I don't really see how to implement this other than with an attribute.

Sorry, that code got formatted strangely. Here's the same code block in more legible form:

MyContainer<T> container;
T t;
while (GetNextT(&t)) {
  container.Add(t);
  if (SomeCondition()) {
    PassToConsumer(std::move(container));
    container.Clear();
  }
}
aaron.ballman accepted this revision.Aug 12 2018, 8:35 AM

LGTM! An attribute really is the right way to go for this sort of thing, and it sounds like you've got a base of users looking for the functionality.

This revision is now accepted and ready to land.Aug 12 2018, 8:35 AM
mboehme updated this revision to Diff 160347.Aug 13 2018, 6:50 AM

Rebase to head.

Thank you for the review!

This revision was automatically updated to reflect the committed changes.