martinboehme wrote:
> @martinboehme Could you explain how that is any different from this?
>
> ```
> #include <string>
> #include <vector>
>
> bool TryInsert(std::vector<std::string>& v, std::string&& s) {
> if (some_condition()) { return false; }
> v.push_back(std::move(s));
> return true;
> }
>
> std::string foo(std::vector<std::string>& v, std::string&& s) {
> if (!TryInsert(v, std::move(s))) {
> return std::move(s); // warning: 's' used after it was moved
> [bugprone-use-after-move]
> }
> return {};
> }
> ```
You mean how is this different from the example I posted earlier? Quoting it
again:
```cxx
SomeMoveableType obj;
void foo(SomeMoveableType &&, SomeOtherType);
// Obviously a contrived example, but you get the point
foo(std::move(obj), obj.operation());
```
One difference is that `TryInsert` in your example has "maybe move" semantics
(it might move, or it might not, similar to `try_emplace()`). But that's not
the decisive difference here.
The key line in my example is this one:
```cxx
foo(std::move(obj), obj.operation());
```
Note how this uses `obj` in two arguments to the same function.
If the check assumed that `std::move(obj)` already leaves `obj` in a moved-from
state, then it would need to flag `obj.operation()` as a use-after-move. But
the check knows that the move only happens once `foo()` starts executing, so
`obj.operation()` is guaranteed to be evaluated before the move, and hence it
is not a use-after-move[^1].
Now let's look at a similar example but with a user-defined invalidation
function. We'll assume
[`close(2)`](https://pubs.opengroup.org/onlinepubs/009604499/functions/close.html)
has been marked as an invalidation function.
```cxx
int fd = open(...);
void foo(int err, ssize_t bytes_written);
foo(close(fd), write(fd, ...));
```
Observations:
- This is a synthetic example -- I'm not sure what purpose this would serve.
But I assume this kind of pattern could come up in real code with other
invalidation functions.
- This is an "use after invalidation". The order in which function arguments
are evaluated is unspecified, so we have to assume pessimistically that
`close(fd)` (the invalidation) is evaluated before `write(fd, ...)` (the use).
- The check does not currently flag this use after invalidation because it uses
the same logic as for `std::move` and hence concludes, erroneously, that the
invalidation only happens once `foo()` starts executing.
> It seems to me the crux of your argument...
>
> > sooner or later someone is going to ~configure an invalidation function~
> > _compose an outer 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.
>
> ...applies ~verbatim to situations with `std::move` and `std::forward` too,
> no?
I'm not sure exactly what you mean here (particularly with respect to the
"compose an outer function"), but I hope I've made it clear above how
`std::move` and `std::forward` are different from user-defined invalidation
functions.
> > I realize this is often not an issue in practice
>
> This is important, and has been our guiding principle here. The reality is
> this check (and the typical tidy in general) has always had rough edges and
> been inherently limited in what it can do.
Definitely. (I'm the original author of bugprone-use-after-move and am
painfully aware of its limitations.)
However, we accept these limitations because they are hard to overcome. The
issue I point out above is not hard to overcome at all: `std::move` and
`std::forward` should continue to determine the `moving-call` using [this
matcher](https://github.com/llvm/llvm-project/blob/a95a9465bfd261f7b7a464d79336faa74af780c0/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp#L508),
whereas user-defined invalidation functions should simply consider the
`moving-call` to be identical to the `call-move`[^2]. (IOW, we should be doing
_less_ work for user-defined invalidation functions!)
My wider point is that this illustrates how the semantics of `std::move` and
`std::forward` are different from user-defined invalidation functions and that
these should therefore be handled by a different check (which would internally
reuse almost all of the implementation).
> All that said, though, I'm not really opposing you: if you'd like to make the
> check more precise, or rename it more accurately, and others are on board --
> I have no particular opposition. I've done what I can to improve it so far,
> and I'll be happy to see contributors improve the check further.
I unfortunately currently can't devote the time to contributing to the check. I
realize it's on me that I didn't notice this PR while it was open and voice my
concerns at the time. I do believe that the status quo is unsatisfactory, and I
hope that I've explained why.
[^1]: C++ doesn't define the order in which function arguments are evaluated;
if `obj.operation()` is evaluated before `std::move(obj)`, there's no question
of this being a use-after-move of course. But even in the case where we get
"unlucky" and the `std::move(obj)` is evaluated first, this isn't a
use-after-move.
[^2]: Note also how these nodes are now misleadingly named. They should really
be `invalidating-call` and `call-invalidator` or similar.
https://github.com/llvm/llvm-project/pull/170346
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
