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

[nlee]

Summary

Returning class or struct by value with const qualifier should be avoided since it prevents move semantics.

clang-tidy has readability-const-return-type that catches this case, but it also impacts move semantics, not just readability.

[lattner]

I'm not a competent reviewer for this, Richard can you recommend someone?

[aaron.ballman]

Thank you for working on this new diagnostic! We don't typically add new diagnostics that are off by default unless they're for pedantic diagnostics or are reasonably expected to be enabled by default in the future. This is because we've had evidence that suggests such diagnostics aren't enabled often enough to warrant the maintenance cost for them.

I'm not convinced this diagnostic can be enabled by default. Yes, it prevents move semantics, but that's not typically an issue with the correctness of the code. IIRC, we decided to put this diagnostic into clang-tidy because we thought it would be too chatty in practice, especially on older code bases.

Perhaps there are ways we can improve the diagnostic behavior to allow it to be enabled by default though. One possibility would be to look at the return type to see if there's a user-provided move constructor (either = default or explicitly written) and only diagnose if one is found. But I think we'd want some evidence that this actually reduces the false positive rate in practice, which means trying to compile some large C++ projects (of various ages/code quality) to see. Would you be willing to run your patch against some large C++ projects to see what kind of true and false positives it generates? From there, we can decide whether we need additional heuristics or not.

[erichkeane]

clang/lib/Sema/SemaType.cpp

5203 ↗

(On Diff #428688)

I wonder if this is same concern applies to Unions? Should this just be T->isRecordType()? Should we do more analysis to ensure that this is a movable type? I THINK CXXRecordDecl::defaultedMoveConstructorIsDeleted() should be enough to test for t his.

[nlee]

In D125402#3508845, @aaron.ballman wrote:

Thank you for working on this new diagnostic! We don't typically add new diagnostics that are off by default unless they're for pedantic diagnostics or are reasonably expected to be enabled by default in the future. This is because we've had evidence that suggests such diagnostics aren't enabled often enough to warrant the maintenance cost for them.

I'm not convinced this diagnostic can be enabled by default. Yes, it prevents move semantics, but that's not typically an issue with the correctness of the code. IIRC, we decided to put this diagnostic into clang-tidy because we thought it would be too chatty in practice, especially on older code bases.

Perhaps there are ways we can improve the diagnostic behavior to allow it to be enabled by default though. One possibility would be to look at the return type to see if there's a user-provided move constructor (either = default or explicitly written) and only diagnose if one is found. But I think we'd want some evidence that this actually reduces the false positive rate in practice, which means trying to compile some large C++ projects (of various ages/code quality) to see. Would you be willing to run your patch against some large C++ projects to see what kind of true and false positives it generates? From there, we can decide whether we need additional heuristics or not.

How about adding CXXRecordDecl::hasMoveConstructor() to ensure the type is movable? I ran a test with OpenCV(c++11), and it returned some useful warnings:

In file included from /Users/nlee/Documents/opencv/modules/core/include/opencv2/core/base.hpp:662:
/Users/nlee/Documents/opencv/modules/core/include/opencv2/core/check.hpp:16:12: warning: returning by const value prevents move semantics [-Wreturn-by-const-value]
CV_EXPORTS const String typeToString(int type);
           ^~~~~~
/Users/nlee/Documents/opencv/modules/core/include/opencv2/core/check.hpp:26:12: warning: returning by const value prevents move semantics [-Wreturn-by-const-value]
CV_EXPORTS const cv::String typeToString_(int type);
           ^~~~~~

@aaron.ballman Can you suggest some other projects to test? I'm thinking of these: protobuf, pytorch

I wonder if this is same concern applies to Unions? Should this just be T->isRecordType()? Should we do more analysis to ensure that this is a movable type? I THINK CXXRecordDecl::defaultedMoveConstructorIsDeleted() should be enough to test for t his.

@erichkeane Does adding CXXRecordDecl::hasMoveConstructor() suffices to ensure the type is movable? I left out unions because they may alert false positives. An example of such a case is:

union U { int i; std::string s; };
const U f() { return U{239}; }

clang/lib/Sema/SemaType.cpp

5203 ↗

(On Diff #428688)

How about adding CXXRecordDecl::hasMoveConstructor() to ensure the type is movable? I left out unions because they may alert false positives. An example of such a case is:

union U { int i; std::string s; };
const U f() { return U{239}; }

5203 ↗

(On Diff #428688)

I wonder if this is same concern applies to Unions? Should this just be T->isRecordType()? Should we do more analysis to ensure that this is a movable type? I THINK CXXRecordDecl::defaultedMoveConstructorIsDeleted() should be enough to test for t his.

[nlee]

Thank you for your reviews, and sorry for the mess in my last comment. I'm new to clang/Phabricator and trying to get used to it :)

clang/lib/Sema/SemaType.cpp

5203 ↗

(On Diff #428688)

It seems it is not always possible to call T->getAsCXXRecordDecl()->hasMoveConstructor() here. CXXRecordDecl::DefinitionData is not populated if definition is not provided in the translation unit, such as in the following case:

extern struct no_def s;
const no_def f();

assert fails with message: "queried property of class with no definition"

[aaron.ballman]

In D125402#3517865, @nlee wrote:

How about adding CXXRecordDecl::hasMoveConstructor() to ensure the type is movable? I ran a test with OpenCV(c++11), and it returned some useful warnings:

I don't think that will be quite correct -- IIRC, that would still return true if the move constructor was deleted. hasSimpleMoveConstructor() and hasSimpleMoveAssignment() might be a better approach.

@aaron.ballman Can you suggest some other projects to test? I'm thinking of these: protobuf, pytorch

Those are good; I'd also run it over LLVM itself. Boost would also be a good thing to test against.

Thank you for your reviews, and sorry for the mess in my last comment. I'm new to clang/Phabricator and trying to get used to it :)

No problem, Phabricator takes some getting used to and the Clang code base is pretty large. :-)

clang/lib/Sema/SemaType.cpp

5203 ↗

(On Diff #428688)

Yeah, we calculate move/copy constructor/assignment (triviality, etc) while building up the type for the class as we add declarations to it. So if we've not seen the definition yet, we won't be able to check that property.

That said, it might make sense to only trigger this diagnostic when *defining* the function because the parameter and return types must be complete by that point.

clang/test/SemaCXX/warn-return-by-const-value.cpp

33 ↗

(On Diff #428688)

You should add the newline to the end of the file.

[erichkeane]

In D125402#3553802, @aaron.ballman wrote:

In D125402#3517865, @nlee wrote:

How about adding CXXRecordDecl::hasMoveConstructor() to ensure the type is movable? I ran a test with OpenCV(c++11), and it returned some useful warnings:

I don't think that will be quite correct -- IIRC, that would still return true if the move constructor was deleted. hasSimpleMoveConstructor() and hasSimpleMoveAssignment() might be a better approach.

The 'Simple' version might not be quite right... That is implemented as:

bool hasSimpleMoveConstructor() const {
   return !hasUserDeclaredMoveConstructor() && hasMoveConstructor() &&
          !data().DefaultedMoveConstructorIsDeleted;
 }

So this would still warn about user-defined move constructors.

HOWEVER, I might suggest hasMoveConstructor() && !defaultedMoveConstructorIsDeleted() for the ctor test. There is similar storage for the 'DefaultedMoveAssignmentIsDeleted`, but it isn't exposed, so you might need to add a function to expose it in DeclCXX.h.

[aaron.ballman]

In D125402#3553825, @erichkeane wrote:

In D125402#3553802, @aaron.ballman wrote:

In D125402#3517865, @nlee wrote:

How about adding CXXRecordDecl::hasMoveConstructor() to ensure the type is movable? I ran a test with OpenCV(c++11), and it returned some useful warnings:

I don't think that will be quite correct -- IIRC, that would still return true if the move constructor was deleted. hasSimpleMoveConstructor() and hasSimpleMoveAssignment() might be a better approach.

The 'Simple' version might not be quite right... That is implemented as:

bool hasSimpleMoveConstructor() const {
   return !hasUserDeclaredMoveConstructor() && hasMoveConstructor() &&
          !data().DefaultedMoveConstructorIsDeleted;
 }

So this would still warn about user-defined move constructors.

Good catch!

HOWEVER, I might suggest hasMoveConstructor() && !defaultedMoveConstructorIsDeleted() for the ctor test. There is similar storage for the 'DefaultedMoveAssignmentIsDeleted`, but it isn't exposed, so you might need to add a function to expose it in DeclCXX.h.

I think that might still not be correct because there could be a user-provided move constructor that's explicitly deleted. (So it has a move constructor, but the defaulted one is not deleted because it's user provided.)

[erichkeane]

In D125402#3553841, @aaron.ballman wrote:

In D125402#3553825, @erichkeane wrote:

In D125402#3553802, @aaron.ballman wrote:

In D125402#3517865, @nlee wrote:

How about adding CXXRecordDecl::hasMoveConstructor() to ensure the type is movable? I ran a test with OpenCV(c++11), and it returned some useful warnings:

I don't think that will be quite correct -- IIRC, that would still return true if the move constructor was deleted. hasSimpleMoveConstructor() and hasSimpleMoveAssignment() might be a better approach.

The 'Simple' version might not be quite right... That is implemented as:

bool hasSimpleMoveConstructor() const {
   return !hasUserDeclaredMoveConstructor() && hasMoveConstructor() &&
          !data().DefaultedMoveConstructorIsDeleted;
 }

So this would still warn about user-defined move constructors.

Good catch!

HOWEVER, I might suggest hasMoveConstructor() && !defaultedMoveConstructorIsDeleted() for the ctor test. There is similar storage for the 'DefaultedMoveAssignmentIsDeleted`, but it isn't exposed, so you might need to add a function to expose it in DeclCXX.h.

I think that might still not be correct because there could be a user-provided move constructor that's explicitly deleted. (So it has a move constructor, but the defaulted one is not deleted because it's user provided.)

I don't think that is possible? I think 'defaultedMoveConstructor' includes the user typing StructName(StructName&&) = delete;.

Note that out of line deleted implementations are prohibited: https://godbolt.org/z/MTK4jGa57 So we don't have to worry about that.

[erichkeane]

In D125402#3553855, @erichkeane wrote:

In D125402#3553841, @aaron.ballman wrote:

In D125402#3553825, @erichkeane wrote:

In D125402#3553802, @aaron.ballman wrote:

In D125402#3517865, @nlee wrote:

How about adding CXXRecordDecl::hasMoveConstructor() to ensure the type is movable? I ran a test with OpenCV(c++11), and it returned some useful warnings:

I don't think that will be quite correct -- IIRC, that would still return true if the move constructor was deleted. hasSimpleMoveConstructor() and hasSimpleMoveAssignment() might be a better approach.

The 'Simple' version might not be quite right... That is implemented as:

bool hasSimpleMoveConstructor() const {
   return !hasUserDeclaredMoveConstructor() && hasMoveConstructor() &&
          !data().DefaultedMoveConstructorIsDeleted;
 }

So this would still warn about user-defined move constructors.

Good catch!

HOWEVER, I might suggest hasMoveConstructor() && !defaultedMoveConstructorIsDeleted() for the ctor test. There is similar storage for the 'DefaultedMoveAssignmentIsDeleted`, but it isn't exposed, so you might need to add a function to expose it in DeclCXX.h.

I think that might still not be correct because there could be a user-provided move constructor that's explicitly deleted. (So it has a move constructor, but the defaulted one is not deleted because it's user provided.)

I don't think that is possible? I think 'defaultedMoveConstructor' includes the user typing StructName(StructName&&) = delete;.

Note that out of line deleted implementations are prohibited: https://godbolt.org/z/MTK4jGa57 So we don't have to worry about that.

I might be wrong about this above:
https://clang.llvm.org/doxygen/classclang_1_1CXXRecordDecl.html#a8ddbc71ffd71a6f7d580fc7a19452f30
Comment says: "
/// Set that we attempted to declare an implicit move

/// constructor, but overload resolution failed so we deleted it."

So perhaps more work to figure out non-deleted move ctor, it might require a lookup. So if we do that, we need a bunch of tests to validate all these conditions I believe (manually deleted, inherited deleted, member-cauesd-deleted, and 'defaulted' versions of the last 2, user provided but works, etc).

[nlee]

After some investigation, I think it is not always explicit that removing const makes any improvements when we are "constructing." For example, in this case, the two codes which differ in the return type of function f (S f() vs. const S f()) produce the same assembly output. I assume this is due to various copy elisions stepping in.

However, when we are "assigning," it makes a difference. In this case, the existence of const in the return type of f decides whether the assignment invokes a move or not.

So I'm suggesting a different policy. Warn if:

• It is an assignment expression AND
• RHS is a function call expression AND
• The function returns by-const-value of a class or struct type AND
• The class or struct is move assignable

The following is a part of diagnosed warnings when applied to llvm:

/Users/namgoolee/Documents/llvm-project/llvm/include/llvm/Option/Option.h:103:9: warning: const qualifying the return value prevents move semantics [-Wpessimizing-return-by-const]
  const Option getGroup() const {
        ^
/Users/namgoolee/Documents/llvm-project/llvm/lib/Option/ArgList.cpp:38:10: note: copy assignment is invoked here even if move assignment is supported for type 'llvm::opt::Option'
       O = O.getGroup()) {
         ^
--
/Users/namgoolee/Documents/llvm-project/clang/include/clang/Lex/MacroInfo.h:421:9: warning: const qualifying the return value prevents move semantics [-Wpessimizing-return-by-const]
  const DefInfo findDirectiveAtLoc(SourceLocation L,
        ^
/Users/namgoolee/Documents/llvm-project/clang/include/clang/Lex/Preprocessor.h:1167:10: note: copy assignment is invoked here even if move assignment is supported for type 'clang::MacroDirective::DefInfo'
      DI = MD->findDirectiveAtLoc(Loc, getSourceManager());
         ^

[nlee]

Updated diff to pass clang-format test.

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

6632

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.)

clang/test/SemaCXX/warn-pessimizing-return-by-const.cpp

51

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).

[nlee]

clang/include/clang/Basic/DiagnosticSemaKinds.td

6632

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.

clang/test/SemaCXX/warn-pessimizing-return-by-const.cpp

51

I'll add those cases.

[nlee]

Warn if:

• It is an assignment expression AND
• RHS is a function call expression AND
• The function returns by-const-value of a class or struct type AND
• The class or struct has non-trivial move assignment operator

Removed DefaultIgnore flag and make check-clang passes.

[cor3ntin]

This has been sitting in the queue for a while, sorry about that.
I think this makes sense in its current iteration, with the warning always on. Have you tried to build a large project like chrome with it?

clang/lib/Sema/SemaChecking.cpp

16843

Maybe we should ignore parentheses too?

16850–16856

you can use CE->getDirectCallee() here

16874

I think a fixit to remove the const would be nice.

clang/include/clang/Basic/DiagnosticGroups.td

 def OrderedCompareFunctionPointers : DiagGroup<"ordered-compare-function-pointers">;
 def Packed : DiagGroup<"packed">;
 def Padded : DiagGroup<"padded">;
 def UnalignedAccess : DiagGroup<"unaligned-access">;
 
 def PessimizingMove : DiagGroup<"pessimizing-move">;
 def ReturnStdMove : DiagGroup<"return-std-move">;
 
+def PessimizingReturnByConst : DiagGroup<"pessimizing-return-by-const">;
+
 def GNUPointerArith : DiagGroup<"gnu-pointer-arith">;
 def PointerArith : DiagGroup<"pointer-arith", [GNUPointerArith]>;
 
 def PoundWarning : DiagGroup<"#warnings">;
 def PoundPragmaMessage : DiagGroup<"#pragma-messages">,
                          DiagCategory<"#pragma message Directive">;
 def : DiagGroup<"redundant-decls">;
 def RedeclaredClassMember : DiagGroup<"redeclared-class-member">;
def Most : DiagGroup<"most", [
     FrameAddress,
     Implicit,
     InfiniteRecursion,
     IntInBoolContext,
     MismatchedTags,
     MissingBraces,
     Move,
     MultiChar,
+    PessimizingReturnByConst,
     RangeLoopConstruct,
     Reorder,
     ReturnType,
     SelfAssignment,
     SelfMove,
     SizeofArrayArgument,
     SizeofArrayDecay,
     StringPlusInt,

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def warn_pessimizing_move_on_return : Warning<
   "moving a local object in a return statement prevents copy elision">,
   InGroup<PessimizingMove>, DefaultIgnore;
 def warn_pessimizing_move_on_initialization : Warning<
   "moving a temporary object prevents copy elision">,
   InGroup<PessimizingMove>, DefaultIgnore;
 def note_remove_move : Note<"remove std::move call here">;
 
+def warn_pessimizing_return_by_const : Warning<
+  "const qualifying the return value prevents move semantics">,
+  InGroup<PessimizingReturnByConst>;
+def note_pessimizing_return_by_const : Note<
+  "copy assignment is invoked here even if move assignment "
+  "is supported for type %0">;
+
 def warn_string_plus_int : Warning<
   "adding %0 to a string does not append to the string">,
   InGroup<StringPlusInt>;
 def warn_string_plus_char : Warning<
   "adding %0 to a string pointer does not append to the string">,
   InGroup<StringPlusChar>;
 def note_string_plus_scalar_silence : Note<
   "use array indexing to silence this warning">;

clang/include/clang/Sema/Sema.h

public:
   /// \p PossibleBody, has a suspicious null statement as a body.
   void DiagnoseEmptyLoopBody(const Stmt *S,
                              const Stmt *PossibleBody);
 
   /// Warn if a value is moved to itself.
   void DiagnoseSelfMove(const Expr *LHSExpr, const Expr *RHSExpr,
                         SourceLocation OpLoc);
 
+  void DiagnosePessimizingConstReturn(const Expr *RHSExpr,
+                                      SourceLocation OpLoc);
+
   /// Warn if we're implicitly casting from a _Nullable pointer type to a
   /// _Nonnull one.
   void diagnoseNullableToNonnullConversion(QualType DstType, QualType SrcType,
                                            SourceLocation Loc);
 
   /// Warn when implicitly casting 0 to nullptr.
   void diagnoseZeroToNullptrConversion(CastKind Kind, const Expr *E);
 

clang/lib/Sema/SemaChecking.cpp

void Sema::DiagnoseSelfMove(const Expr *LHSExpr, const Expr *RHSExpr,
   }
 
   if (isa<CXXThisExpr>(LHSBase) && isa<CXXThisExpr>(RHSBase))
     Diag(OpLoc, diag::warn_self_move) << LHSExpr->getType()
                                         << LHSExpr->getSourceRange()
                                         << RHSExpr->getSourceRange();
 }
 
+void Sema::DiagnosePessimizingConstReturn(const Expr *RHSExpr,
+                                          SourceLocation OpLoc) {
+
+  if (!getLangOpts().CPlusPlus11 || inTemplateInstantiation())
+    return;
+
+  RHSExpr = RHSExpr->IgnoreImplicit();
+
+  // Is RHS a function call expression?
+  const CallExpr *CE = dyn_cast<const CallExpr>(RHSExpr);
+  if (!CE)
+    return;
+
+  const Decl *CD = CE->getCalleeDecl();
+  if (!CD)
+    return;
+
+  const FunctionDecl *FD = dyn_cast<const FunctionDecl>(CD);
+  if (!FD)
+    return;
+
+  // Is the return type by-const-value of a struct or class type?
+  const QualType RT = FD->getReturnType();
+  if (!RT->isStructureOrClassType() || !RT.isConstQualified())
+    return;
+
+  // Is the move assignment operator explicitly deleted?
+  bool MoveAssignmentIsDeleted = false;
+  for (const CXXMethodDecl *iter : RT->getAsCXXRecordDecl()->methods())
+    if (iter->isMoveAssignmentOperator() && iter->isDeleted())
+      MoveAssignmentIsDeleted = true;
+
+  if (RT->getAsCXXRecordDecl()->hasDefinition() &&
+      RT->getAsCXXRecordDecl()->hasMoveAssignment() &&
+      RT->getAsCXXRecordDecl()->hasNonTrivialMoveAssignment() &&
+      !MoveAssignmentIsDeleted) {
+    const SourceRange ReturnTypeLoc = FD->getReturnTypeSourceRange();
+    Diag(ReturnTypeLoc.getBegin(), diag::warn_pessimizing_return_by_const);
+    Diag(OpLoc, diag::note_pessimizing_return_by_const)
+        << RT.getUnqualifiedType();
+  }
+}
+
 //===--- Layout compatibility ----------------------------------------------//
 
 static bool isLayoutCompatible(ASTContext &C, QualType T1, QualType T2);
 
 /// Check if two enumeration types are layout-compatible.
 static bool isLayoutCompatible(ASTContext &C, EnumDecl *ED1, EnumDecl *ED2) {
   // C++11 [dcl.enum] p8:
   // Two enumeration types are layout-compatible if they have the same

clang/lib/Sema/SemaOverload.cpp

case OR_Success: {
         ArrayRef<const Expr *> ArgsArray(Args, 2);
         const Expr *ImplicitThis = nullptr;
         // Cut off the implicit 'this'.
         if (isa<CXXMethodDecl>(FnDecl)) {
           ImplicitThis = ArgsArray[0];
           ArgsArray = ArgsArray.slice(1);
         }
 
-        // Check for a self move.
-        if (Op == OO_Equal)
+        if (Op == OO_Equal) {
           DiagnoseSelfMove(Args[0], Args[1], OpLoc);
+          DiagnosePessimizingConstReturn(Args[1], OpLoc);
+        }
 
         if (ImplicitThis) {
           QualType ThisType = Context.getPointerType(ImplicitThis->getType());
           QualType ThisTypeFromDecl = Context.getPointerType(
               cast<CXXMethodDecl>(FnDecl)->getThisObjectType());
 
           CheckArgAlignment(OpLoc, FnDecl, "'this'", ThisType,
                             ThisTypeFromDecl);

clang/test/Misc/warning-wall.c

 CHECK-NEXT:    -Wmismatched-tags
 CHECK-NEXT:    -Wmissing-braces
 CHECK-NEXT:    -Wmove
 CHECK-NEXT:      -Wpessimizing-move
 CHECK-NEXT:      -Wredundant-move
 CHECK-NEXT:      -Wreturn-std-move
 CHECK-NEXT:      -Wself-move
 CHECK-NEXT:    -Wmultichar
+CHECK-NEXT:    -Wpessimizing-return-by-const
 CHECK-NEXT:    -Wrange-loop-construct
 CHECK-NEXT:    -Wreorder
 CHECK-NEXT:      -Wreorder-ctor
 CHECK-NEXT:      -Wreorder-init-list
 CHECK-NEXT:    -Wreturn-type
 CHECK-NEXT:      -Wreturn-type-c-linkage
 CHECK-NEXT:    -Wself-assign
 CHECK-NEXT:      -Wself-assign-overloaded

clang/test/SemaCXX/warn-pessimizing-return-by-const.cpp

+// RUN: %clang_cc1 -fsyntax-only -Wpessimizing-return-by-const -std=c++11 -verify %s
+
+// A class with non-trivial move assignment operator
+struct NT1 {
+  char *d;
+  NT1() {
+    d = new char[1];
+    d[0] = '\0';
+  }
+
+  NT1(const char *c_str) {
+    const int to_alloc = length(c_str) + 1;
+    d = new char[to_alloc];
+    copy(d, c_str);
+  }
+
+  ~NT1() {
+    delete[] d;
+  }
+
+  NT1 &operator=(NT1 &&rhs) {
+    if (this == &rhs)
+      return *this;
+    delete[] d;
+    d = rhs.d;
+    rhs.d = new char[1];
+    rhs.d[0] = '\0';
+    return *this;
+  }
+
+  NT1(const NT1 &rhs) {
+    const int to_alloc = length(rhs.d) + 1;
+    d = new char[to_alloc];
+    copy(d, rhs.d);
+  }
+
+  NT1 &operator=(const NT1 &rhs) {
+    if (this == &rhs)
+      return *this;
+    delete[] d;
+    const int to_alloc = length(rhs.d) + 1;
+    d = new char[to_alloc];
+    copy(d, rhs.d);
+    return *this;
+  }
+
+  // c_str : null-terminated c string. If empty, c_str[0] is '\0'. Cannot be nullptr.
+  // Returns the length of the string, excluding the null-terminating byte.
+  // If given an empty string, returns 0.
+  static int length(const char *c_str) {
+    int ret = 0;
+    while (*c_str++)
+      ++ret;
+    return ret;
+  }
+
+  // dst : must be pre-allocated
+  // Copies src to dst, including the null-terminator.
+  static void copy(char *dst, const char *src) {
+    while ((*dst++ = *src++))
+      ;
+  }
+
+  bool operator==(const NT1 &rhs) {
+    if (length(d) != length(rhs.d))
+      return false;
+    int cnt = 0;
+    const char *s1 = d;
+    const char *s2 = rhs.d;
+    while (cnt++ < length(d))
+      if (*s1++ != *s2++)
+        return false;
+    return true;
+  }
+};
+
+NT1 f_nt1() {
+  return NT1{"f_nt1"};
+}
+
+const NT1 f_nt1_const() { // expected-warning{{const qualifying the return value prevents move semantics}}
+  return NT1{"f_nt1_const"};
+}
+
+void run_nt1() {
+  NT1 nt1;
+  nt1 = f_nt1();
+  nt1 = f_nt1_const(); // expected-note{{copy assignment is invoked here even if move assignment is supported for type 'NT1'}}
+}
+
+// NT2 has non-trivial move assignment operator since the move assignment operator selected
+// to move the base class NT1 is non-trivial
+struct NT2 : public NT1 {
+  NT2() : NT1() {}
+  NT2(const char *c_str) : NT1(c_str) {}
+};
+
+NT2 f_nt2() {
+  return NT2{"f_nt2"};
+}
+
+const NT2 f_nt2_const() { // expected-warning{{const qualifying the return value prevents move semantics}}
+  return NT2{"f_nt2_const"};
+}
+
+void run_nt2() {
+  NT2 nt2;
+  nt2 = f_nt2();
+  nt2 = f_nt2_const(); // expected-note{{copy assignment is invoked here even if move assignment is supported for type 'NT2'}}
+}
+
+// NT3 has non-trivial move assignment operator since the non-static data member of NT1
+// has non-trivial move assignment operator
+struct NT3 {
+  NT1 nt1;
+  NT3() : nt1(NT1{}) {}
+  NT3(const char *c_str) : nt1(NT1(c_str)) {}
+
+  bool operator==(const NT3 &rhs) {
+    return nt1 == rhs.nt1;
+  }
+};
+
+NT3 f_nt3() {
+  return NT3{"f_nt3"};
+}
+
+const NT3 f_nt3_const() { // expected-warning{{const qualifying the return value prevents move semantics}}
+  return NT3{"f_nt3_const"};
+}
+
+void run_nt3() {
+  NT3 nt3;
+  nt3 = f_nt3();
+  nt3 = f_nt3_const(); // expected-note{{copy assignment is invoked here even if move assignment is supported for type 'NT3'}}
+}
+
+// NT4 has defaulted move assignment operator which is still non-trivial
+struct NT4 {
+  NT1 nt1;
+  NT4() : nt1(NT1{}) {}
+  NT4(const char *c_str) : nt1(NT1(c_str)) {}
+
+  NT4 &operator=(NT4 &&) = default;
+
+  NT4(const NT4 &) = default;
+  NT4 &operator=(const NT4 &) = default;
+
+  bool operator==(const NT4 &rhs) {
+    return nt1 == rhs.nt1;
+  }
+};
+
+NT4 f_nt4() {
+  return NT4{"f_nt4"};
+}
+
+const NT4 f_nt4_const() { // expected-warning{{const qualifying the return value prevents move semantics}}
+  return NT4{"f_nt4_const"};
+}
+
+void run_nt4() {
+  NT4 nt4;
+  nt4 = f_nt4();
+  nt4 = f_nt4_const(); // expected-note{{copy assignment is invoked here even if move assignment is supported for type 'NT4'}}
+}
+
+// NT5 have deleted move assignment operator. We do not warn in this case
+struct NT5 {
+  NT1 nt1;
+  NT5() : nt1(NT1{}) {}
+  NT5(const char *c_str) : nt1(NT1(c_str)) {}
+
+  NT5 &operator=(NT5 &&) = delete;
+
+  NT5(const NT5 &) = default;
+  NT5 &operator=(const NT5 &) = default;
+
+  bool operator==(const NT5 &rhs) {
+    return nt1 == rhs.nt1;
+  }
+};
+
+const NT5 f_nt5_const() {
+  return NT5{"f_nt5_const"};
+}
+
+void run_nt5() {
+  NT5 nt5;
+  nt5 = f_nt5_const();
+}
+
+// NT6 has deleted move assignment operator because its member(NT5) has deleted move assignment operator.
+// We do not warn in this case.
+struct NT6 {
+  NT5 nt5;
+};
+
+const NT6 f_nt6_const() {
+  return NT6{"f_nt6_const"};
+}
+
+void run_nt6() {
+  NT6 nt6;
+  nt6 = f_nt6_const();
+}
+
+// warning is suppressed for trivial move assignment operator
+
+struct T1 {
+  int i1, i2, i3, i4;
+};
+
+const T1 f_t1_const() {
+  return T1{};
+}
+
+void run_t1() {
+  T1 t1;
+  t1 = f_t1_const();
+}
+
+struct T2 {
+  T2() = default;
+  T2(const T2 &) = default;
+  T2 &operator=(const T2 &) = default;
+  T2 &operator=(T2 &&) = default;
+};
+
+const T2 f_t2_const() {
+  return T2{};
+}
+
+void run_t2() {
+  T2 t2;
+  t2 = f_t2_const();
+}
+
+struct T3 {
+  T3() = default;
+  T3(const T3 &) = default;
+  T3 &operator=(const T3 &) = default;
+};
+
+const T3 f_t3_const() {
+  return T3{};
+}
+
+void run_t3() {
+  T3 t3;
+  t3 = f_t3_const();
+}
+
+struct T4 {
+  T4() = default;
+  T4(const T4 &) = default;
+  T4 &operator=(const T4 &) = default;
+  T4 &operator=(T4 &&) = delete;
+};
+
+const T4 f_t4_const() {
+  return T4{};
+}
+
+void run_t4() {
+  T4 t4;
+  t4 = f_t4_const();
+}
+
+struct T5 {
+  T1 t1;
+};
+
+const T5 f_t5_const() {
+  return T5{};
+}
+
+void run_t5() {
+  T5 t5;
+  t5 = f_t5_const();
+}
+
+struct T6 : public T1 {
+};
+
+const T6 f_t6_const() {
+  return T6{};
+}
+
+void run_t6() {
+  T6 t6;
+  t6 = f_t6_const();
+}