[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Skylion007 added a comment. In D137205#4054534 , @Febbe wrote: > In D137205#4047828 , @Skylion007 > wrote: > >> I am trying this in the wild and getting some false positives where it tries >> to call std::move i

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Skylion007 added a comment. I am trying this in the wild and getting some false positives where it tries to call std::move inside loop conditions and in the boolean condition for an if statement. Stuff like: if (auto* new_ptr = steal_ptr(std::move(old_ptr))) { ... } else { return

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Skylion007 added a comment. Pinging @njames93 to about the questions from the previous comment. Would love to see something like this PR get merged. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137205/new/ https://reviews.llvm.org/D137205 __

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Skylion007 requested changes to this revision. Skylion007 added a comment. This revision now requires changes to proceed. Typo Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp:227 + + auto const IsMoveAssingable = cxxOperatorCallExpr( +

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Skylion007 added a comment. The main false positive I also keep seeing is in pybind11 where it suggests call an std::move() for an implicit conversion, but the target of the implicit conversion does not have an rvalue. (In this case, we have a py::object which inherits from py::handle, and is j

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Skylion007 added a comment. Hmm, I still had a crash on this latest code (although it is crashing much more rarely now. It crashes on scanning this file https://github.com/facebookresearch/habitat-sim/blob/5fb04078b0b8432dc4a88ec186a2b7af74163be1/src/esp/core/managedContainers/ManagedContainerB

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Skylion007 added a comment. Currently, this check also tries to move static values which is very undesirable and unlikely to be correct. static auto value = std::string("CONSTANT"); - return value; + return std::move(value); Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Skylion007 added a comment. Okay, now I am getting what I believe to be segfaults: #0 0x564383482be4 PrintStackTraceSignalHandler(void*) Signals.cpp:0:0 #1 0x564383480464 SignalHandler(int) Signals.cpp:0:0 #2 0x7f7c275c9420 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Skylion007 added a comment. In D137205#3913192 , @Febbe wrote: > In D137205#3912243 , @Skylion007 > wrote: > >> One other bug I found with this diff, is it seems to suggest calling >> std::move() on function arg

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Skylion007 added a comment. One other bug I found with this diff, is it seems to suggest calling std::move() on function args that are references, despite the fact that invalidating the reference to the input arg could be undesirable. For instance take a function that takes in a reference to a

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Skylion007 added a comment. I noticed one other bug from testing this out on some C++ codebases. Specifically in pybind11 we have an object class which is a child of the handle object. The object class is non-trivially copyable, while the handle object is not. Therefore, when we do not want to

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Skylion007 added a comment. Use llvm::find_if instead of std::find_if Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137205/new/ https://reviews.llvm.org/D137205 ___ cfe-commits mailing list cfe-commits@l