PiotrZSL added a comment.
Overall looks ok,
Add some test with records being used via typedef.
================
Comment at: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:117
+ bool IsConstArg = ArgType.isConstQualified();
+ bool IsTriviallyCopyable = ArgType.isTriviallyCopyableType(*Result.Context);
----------------
make sure you use here canonical type, or it may not work correctly with
typedefs, like typedefs to records.
same in line 120, in fact only on line 154 would be good to use original type.
================
Comment at: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:209
+ !RecordDecl->hasMoveConstructor() || !RecordDecl->hasMoveAssignment())
+ diag(RecordDecl->getSourceRange().getBegin(),
+ "'%0' is not move constructible", DiagnosticIDs::Note)
----------------
`getLocation ()`
================
Comment at: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:211
+ "'%0' is not move constructible", DiagnosticIDs::Note)
+ << RecordDecl->getName();
}
----------------
`<< RecordDecl`
should work, do not use getName, it could cause crash for unnamed records.
================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:378
+- Improved :doc:`performance-move-const-arg
+ <clang-tidy/checks/performance/move-const-arg>`: warning when no move
+ constructor is available.
----------------
"check to warn when ..."
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/performance/move-const-arg.cpp:345
+namespace issue_62550 {
+
+struct NonMoveConstructable {
----------------
AMS21 wrote:
> I've tried to add
>
> ```
> // CHECK-MESSAGES: [[@LINE+1]]:1: note: 'NonMoveConstructable' is not move
> constructible
> ```
>
> here. But doing so actually fails the tests and I'm not quite sure why.
There are 2 ways to deal with this.
If you use CHECK-MESSAGES, then order of issues is checked, and that why for
current notes you may see thing like `@LINE-27`, so you would need to put it
into places when its emited.
Other way is to use CHECK-NOTES, then all warnings are checked out of order.
So simply I would put those CHECK-MESSAGES, after a warning in lines 256, 372,
384, 387
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
