nlee added inline comments.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6632
+ "const qualifying the return value prevents move semantics">,
+ InGroup<PessimizingReturnByConst>, DefaultIgnore;
+def note_pessimizing_return_by_const : Note<
----------------
aaron.ballman wrote:
> Given that this is a `DefaultIgnore`, I'm still not quite sure this belongs
> in Clang rather than continuing to be covered by clang-tidy.
>
> If you leave it on by default, how many Clang tests break as a result? Do the
> new warnings all look like true positives? (Basically, I'm trying to see how
> far away we are from being able to enable this diagnostic by default.)
> how many Clang tests break as a result?
All Clang tests pass without breaking.
> Do the new warnings all look like true positives?
When applied to Clang source code, it does catch some cases as programmed. One
problem I see is that it also catches for classes with only primitive member
variables where move assignment(which is copy) has no advantage over copy
assignment.
================
Comment at: clang/test/SemaCXX/warn-pessimizing-return-by-const.cpp:50
+
+const S3 f3_const() { // do not warn if move assignment operator is deleted
+ return S3{};
----------------
aaron.ballman wrote:
> We should also have a test for an explicitly deleted move assignment, and
> another one where a member of the class is not movable (and thus the move
> assignment should be deleted that way as well).
I'll add those cases.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125402/new/
https://reviews.llvm.org/D125402
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
