[PATCH] D125402: [clang][diag] warn if function returns class type by-const-value

[Namgoo Lee via Phabricator via cfe-commits]

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