PiotrZSL accepted this revision. PiotrZSL added a comment. This revision is now accepted and ready to land.
+- LGTM ================ Comment at: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:207 + + if (const CXXRecordDecl *RecordDecl = ArgType->getAsCXXRecordDecl(); + !RecordDecl->hasMoveConstructor() || !RecordDecl->hasMoveAssignment()) ---------------- You need t check RecordDecl here also. like `if (...; RecordDecl && !(RecordDecl->hasMoveConstructor() && RecordDecl->hasMoveAssignment()))` ================ Comment at: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:209 + !RecordDecl->hasMoveConstructor() || !RecordDecl->hasMoveAssignment()) + diag(RecordDecl->getLocation(), "%0 is not move constructible", + DiagnosticIDs::Note) ---------------- maybe this should be `constructible/assignable` depend on what we are missing (constructor, assign operator)... Simply this may be strange to get warn about construct when we assign. Still things like std::optional cold use move constructor instead of assign operator on assign. So this wound need to be checked... It's fine if we won't be very detailed here ================ Comment at: clang-tools-extra/docs/ReleaseNotes.rst:378 +- Improved :doc:`performance-move-const-arg + <clang-tidy/checks/performance/move-const-arg>`: check to warn when no move + constructor is available. ---------------- remove `:` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153220/new/ https://reviews.llvm.org/D153220 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
