[PATCH] D145581: [clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments.

[Martin Böhme via Phabricator via cfe-commits]

mboehme added a comment.

Sorry for the delay in replying -- I was caught up in other projects.



================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:224
+  <clang-tidy/checks/bugprone/use-after-move>`:
+  - Fixed handling for designated initializers.
+  - Fix: In C++17 and later, the callee of a function is guaranteed to be
----------------
Reworded according to comments in https://reviews.llvm.org/D145906.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp:1296
 
+// In a function call, the expression that determines the callee is sequenced
+// before the arguments.
----------------
PiotrZSL wrote:
> This entire file tests only C++17 and later, when std::move is C++11 feature.
> Add separate test file for C++11 (or even better split this one between C++11 and C++17)
> 
> Scenario to test in C++11, should warn ?
> ```
> a->foo(std::move(a));
> ```
> This entire file tests only C++17 and later, when std::move is C++11 feature.
> Add separate test file for C++11 (or even better split this one between C++11 and C++17)

Good point. I've added a new `RUN` line that also runs this in C++11 mode.

> Scenario to test in C++11, should warn ?

See below -- this does produce a warning in C++11 mode (but not C++17 mode).

I've also redone the tests to reuse the existing class `A` without having to define a new one.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp:1304
+  std::unique_ptr<A> a;
+  a->foo(std::move(a));
+}
----------------
PiotrZSL wrote:
> What about scenario like this:
> 
> ```
> b.foo(a->saveBIntoAAndReturnBool(std::move(b)));
> ```
> 
> Is first "b" still guaranteed to be alive after std::move ?
I'm not exactly sure what you're asking here... or how this scenario is materially different from the other scenarios we already have?

> Is first "b" still guaranteed to be alive after std::move ?

The `b` in `b.foo` is guaranteed to be evaluated before the call `a->saveBIntoAAndReturnBool(std::move(b))` -- but I'm not sure if this is what you're asking?

Or are you asking whether the `a->saveBIntoAAndReturnBool(std::move(b))` can cause the underlying object to be destroyed before the call to `b.foo` happenss? In other words, do we potentially have a use-after-free here?

I think the answer to this depends on what exactly `saveBIntoAAndReturnBool()` does (what was your intent here?). I also think it's probably beyond the scope of this check in any case, as this check is about diagnosing use-after-move, not use-after-free.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp:1305
+  a->foo(std::move(a));
+}
+} // namespace CalleeSequencedBeforeArguments
----------------
PiotrZSL wrote:
> I didn't found any test for correct warning in this case:
> 
> ```
> std::unique_ptr<A> getArg(std::unique_ptr<A>);
> getArg(std::move(a))->foo(std::move(a));
> ```
Good point -- added below (in slightly different form, but testing the same thing).

Note that this is an error in both C++11 and C++17, but in C++11 the use and move are unsequenced, whereas in C++17 the use definitely comes after the move.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145581/new/

https://reviews.llvm.org/D145581