[PATCH] D153220: [clang-tidy] Improve `performance-move-const-arg` message when no move constructor is available

[Piotr Zegar via Phabricator via cfe-commits]

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