[clang-tools-extra] Extend bugprone-use-after-move check to handle std::optional::reset() and std::any::reset() (PR #114255)

Julian Schmidt via cfe-commits cfe-commits at lists.llvm.org
Sat Nov 2 04:06:51 PDT 2024


================
@@ -279,7 +281,7 @@ void UseAfterMoveFinder::getDeclRefs(
         if (DeclRef && BlockMap->blockContainingStmt(DeclRef) == Block) {
           // Ignore uses of a standard smart pointer that don't dereference the
           // pointer.
-          if (Operator || !isStandardSmartPointer(DeclRef->getDecl())) {
+          if (Operator || !isStandardResettableOwner(DeclRef->getDecl())) {
----------------
5chmidti wrote:

> `std::optional::has_value` seems OK (it's no different than `operator==(const std::unique_ptr &, std::nullptr_t)`, both of which are OK as they don't actually access the moved-from value)+
[...]
I'm kind of torn here. I'd much rather prioritize C++17 over C++23, since I expect `reset` to be used much more, but the monadic operators might eventually become commonplace too.

Actually, it is for `optional` and `any` (though `any` is left in a fully unspecified state, unlike `optional`). It's not just the monadic operators, but e.g., `has_value` together with `value` (etc.): https://godbolt.org/z/hc4MEPWj8

```c++
#include <iostream>
#include <optional>
#include <vector>

void sink(std::optional<std::vector<int>> b) {
    if (b.has_value()) {
        std::cout << "moved to: " << b.value().front() << '\n' << std::flush;
    }
}

int main() {
    auto a = std::optional<std::vector<int>>{{42}};
    sink(std::move(a));

    if (a.has_value()) { // moved-from optional `a` still returns true here
        // accessing moved from contained object
        std::cout << "moved from: " << a.value().front() << '\n' << std::flush;
    }
}
```

Both post-move accesses would not be considered as accessing the contained value due to the focus on the dereferencing operators of smart pointers.

> But then again, it seems(?) the fact that we don't warn on `operator==` is just an accident caused by it being a non-member, not intentional?

All operations except `operator*`, `operator->` or `operator[]` are completely fine from what I can tell. The ptr of smart pointers is set to `nullptr`, which allows for things like `operator==` or `operator bool` to be valid uses, because unlike `optional` they don't return `true` when the internal object has been moved from/is a `nullptr` (`weak_ptr` is different, but has similar semantics to the other smart pointers).

> I guess, in theory, what we need to do here is to distinguish the different methods [...]
> Barring that, a blanket rule should either allow ~all members, or prohibit ~all members.

For `optional` and `any` I'd say we swap the logic around and only allow `reset`, `emplace` and assignments (already exists) as resetting the state, and warn on anything else, while the smart_ptr case only disallows the operators it does right now.

> but I don't really have the bandwidth to try to figure out how to implement that...

I would've said that this PR should be blocked until the above is handled correctly, but actually, the PR as is, is removing some false-positives by considering the calls to `reset` as resetting the state. So there are no real negatives attached to this PR, that weren't there before, it's only improving the state. The changes outlined above make some negatives positives, which could be done in a follow-up (+issue in case you do not end up implementing it) (in one go would be nicer, of course).

https://github.com/llvm/llvm-project/pull/114255


More information about the cfe-commits mailing list