martinboehme wrote: @higher-performance > Yes, these were explicitly specified. The error messages themselves aren't > necessarily confusing, it's just that the name of the check also shows up, > and people assume there has to be a move because the name of the check has > move in it, when the invalidation occurs due to something else like close() > or free().
Exactly. We have an internal configuration where `close()` is configured as one of the invalidation functions, and the developer encountering the error message got confused because "bugprone-use-after-move" has "move" in it, yet they didn't see a move in their code. @vbvictor > I'm -1 on separating this checks because we essentially will duplicate a huge > portion of logic into multiple places which can double the time of analysis Separating the checks would not double the time spent on analysis. We run a separate analysis for every "moving call" that we encounter -- see the the matcher [here](https://github.com/llvm/llvm-project/blob/a95a9465bfd261f7b7a464d79336faa74af780c0/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp#L508). So the number of analyses we run would remain the same: One for every call to an invalidation function. > and we need to keep two versions in sync. All of the shared code should, of course, be in a shared location and should not be duplicated. @higher-performance > I'm not really sure what to do about this given the check is basically > identical regardless of the name of the function. There's an important way in which the check should _not_ be the same for `std::move` versus other invalidation functions. The check considers the `Stmt` that performs the move to be not the `std::move` itself but its parent (with some nodes being ignored -- for details, see [this matcher](https://github.com/llvm/llvm-project/blob/a95a9465bfd261f7b7a464d79336faa74af780c0/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp#L508)). This reflects the fact that `std::move` itself does not perform the move but just a cast, which can become important in situations like this: ```cxx SomeMoveableType obj; void foo(SomeMoveableType &&, SomeOtherType); // Obviously a contrived example, but you get the point foo(std::move(obj), obj.operation()); ``` This isn't a use-after-move (regardless of the order in which arguments are evaluated) because the move only happens inside `foo()` and is thus guaranteed to happen after the evaluation of `obj.operation()`. (Never mind that this example seems a bit pointless because `foo()` itself could call `obj.operation()` -- but I've seen real code do more complex variations of this kind of pattern.) For other invalidation functions, however, we _should_ consider the call to the invalidation function itself to be the invalidation, not its parent. I realize this is often not an issue in practice because I presume many invalidation functions return `void`, but sooner or later someone is going to configure an invalidation function that does return a value, will use that value in an expression, and be mightily confused about why the check doesn't understand when the invalidation happens. Fixing this obviously doesn't require splitting the check into two separate checks, but it shows how "move" is different from other forms of "invalidation" and is therefore another reason to handle these in two different checks to avoid programmer confusion. https://github.com/llvm/llvm-project/pull/170346 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
