clang: calls redundant move-constructor when explicit constructor of a nested object is called in an aggregate

[samolisov]

To be short

When clang compiles the following minimum demonstration with an aggregate with a base class where the base class has an explicit constructor and therefore double {} cannot be used to make the aggregate's instances, a code to create a temporary object of the base class is generated by clang and then the move constructor is invoked redundancy to move the temporary into its correct place within the aggregate. I compared the code with the one generated by MSVC and g++, they do not insert a call for the move constructor (but to be clear, I've found a comment in the corresponding code of clang what unsure me who does the correct thing in this situation).

The code:

#include <cstdio>

struct A {
  /*explicit */A(const char *) { printf("A(const char *)\n"); }
  A(const A &) { printf("A(const A &)\n"); }
  A(A&&) { printf("A(A&&)\n"); }
  A &operator=(const A &) { printf("operator=(const A&)\n"); return *this; }
  A &operator=(A &&) { printf("operator=(A&&)\n"); return *this; }
  ~A() { printf("~A()\n"); }
};

struct B : A {};

B rvo() {
  return B{{"Hi"}};
}

B norvo() {
  return B{A{"Hi2"}};
}

A rvoa() {
  return {"hi"};
}

A norvoa() {
  return A{"hi2"};
}

int main() {
  {
    printf("rvo:\n");
    B b = rvo();
    A a = rvoa();
  }

  {
    printf("no rvo:\n");
    B b = norvo();
    A a = norvoa();
  }

  return 0;
}

So, when an object of type B is created using B{{"Hi"}}, everything is fine and no move-constructor for A is invoked, but when the object of B is created using B{A{"Hi2"}}, the move constructor for A is invoked.

Analysis

The following AST was generated for the first case (B{{"Hi"}}):

|-FunctionDecl 0x28607286238 <line:14:1, line:16:1> line:14:3 used rvo 'B ()'
| `-CompoundStmt 0x28607286620 <col:9, line:16:1>
|   `-ReturnStmt 0x28607286610 <line:15:3, col:18>
|     `-ExprWithCleanups 0x286072865f8 <col:10, col:18> 'B':'B'
|       `-CXXFunctionalCastExpr 0x286072865d0 <col:10, col:18> 'B':'B' functional cast to B <NoOp>
|         `-CXXBindTemporaryExpr 0x286072865b0 <col:11, col:18> 'B':'B' (CXXTemporary 0x286072865b0)
|           `-InitListExpr 0x28607286418 <col:11, col:18> 'B':'B'
|             `-CXXConstructExpr 0x28607286568 <col:12, col:17> 'A':'A' 'void (const char *)' list
|               `-ImplicitCastExpr 0x28607286460 <col:13> 'const char *' <ArrayToPointerDecay>
|                 `-StringLiteral 0x28607286368 <col:13> 'const char[3]' lvalue "Hi"

while the AST for the second case (B{A{"Hi2"}}) contains a MaterializeTemporaryExpr expression:

|-FunctionDecl 0x28607286658 <line:18:1, line:20:1> line:18:3 used norvo 'B ()'
| `-CompoundStmt 0x286072869c8 <col:11, line:20:1>
|   `-ReturnStmt 0x286072869b8 <line:19:3, col:20>
|     `-ExprWithCleanups 0x286072869a0 <col:10, col:20> 'B':'B'
|       `-CXXFunctionalCastExpr 0x28607286978 <col:10, col:20> 'B':'B' functional cast to B <NoOp>
|         `-CXXBindTemporaryExpr 0x28607286958 <col:11, col:20> 'B':'B' (CXXTemporary 0x28607286958)
|           `-InitListExpr 0x286072868c0 <col:11, col:20> 'B':'B'
|             `-CXXConstructExpr 0x28607286920 <col:12, col:19> 'A':'A' 'void (A &&)'
|               `-MaterializeTemporaryExpr 0x28607286908 <col:12, col:19> 'A':'A' xvalue
|                 `-CXXBindTemporaryExpr 0x28607286858 <col:12, col:19> 'A':'A' (CXXTemporary 0x28607286858)
|                   `-CXXTemporaryObjectExpr 0x28607286818 <col:12, col:19> 'A':'A' 'void (const char *)' list
|                     `-ImplicitCastExpr 0x28607286800 <col:14> 'const char *' <ArrayToPointerDecay>
|                       `-StringLiteral 0x28607286798 <col:14> 'const char[4]' lvalue "Hi2"

After some debugging, I've found the following code in the SemaInit.cpp file, which is responsible for adding a QualificationConversion step into the sequence of initialization actions:

  // C++17 [dcl.init]p17:
  //     - If the initializer expression is a prvalue and the cv-unqualified
  //       version of the source type is the same class as the class of the
  //       destination, the initializer expression is used to initialize the
  //       destination object.
  // Per DR (no number yet), this does not apply when initializing a base
  // class or delegating to another constructor from a mem-initializer.
  // ObjC++: Lambda captured by the block in the lambda to block conversion
  // should avoid copy elision.
  if (S.getLangOpts().CPlusPlus17 && !RequireActualConstructor &&
      UnwrappedArgs.size() == 1 && UnwrappedArgs[0]->isPRValue() &&
      S.Context.hasSameUnqualifiedType(UnwrappedArgs[0]->getType(), DestType)) {
    // Convert qualifications if necessary.
    Sequence.AddQualificationConversionStep(DestType, VK_PRValue);
    ...
    return;
  }
  ...

But this code works only for the first case because only in this case the RequireActualConstructor variable is set to false. In the second case, with an explicit constructor call, the variable is set to true and no-early return occurs, the rest of the void TryConstructorInitialization() function is executed what ends to the adding a ConstructorInitialization step into the sequence of initialization actions:

  // Add the constructor initialization step. Any cv-qualification conversion is
  // subsumed by the initialization.
  Sequence.AddConstructorInitializationStep(
      Best->FoundDecl, CtorDecl, DestArrayType, HadMultipleCandidates,
      IsListInit | IsInitListCopy, AsInitializerList);

The RequireActualConstructor variable is set by the following expression:

bool RequireActualConstructor =
      !(Entity.getKind() != InitializedEntity::EK_Base &&
        Entity.getKind() != InitializedEntity::EK_Delegating &&
        Entity.getKind() !=
            InitializedEntity::EK_LambdaToBlockConversionBlockElement);

In this case, we have Entity.getKind() equals to InitializedEntity::EK_Base. When the decision for the RequireActualConstructor variable is changed (this comparison was excluded), the observed behavior is changed too and no calling of the move constructor is generated anymore.

This may look as a fix until we see the second part of the comment on line 4264:

  // Per DR (no number yet), this does not apply when initializing a base
  // class or delegating to another constructor from a mem-initializer.

I didn't manage to find the corresponding DR, as I see this is now (hm, now, I mean 7 years ago?) a part of the C++17 standard as well as all the subsequent versions and before changing anything in the code the question about legality of such change should be considered. But the difference in the behavior between clang and MSVC + gcc makes me wonder.

My concern is the following, in the part of creating A we are creating an instance of A, not B and it is not clear why Entity is set to InitializedEntity::EK_Base.

[llvmbot]

@llvm/issue-subscribers-clang-frontend

Author: Pavel Samolysov (samolisov)

**To be short**

When clang compiles the following minimum demonstration with an aggregate with a base class where the base class has an explicit constructor and therefore double {} cannot be used to make the aggregate's instances, a code to create a temporary object of the base class is generated by clang and then the move constructor is invoked redundancy to move the temporary into its correct place within the aggregate. I compared the code with the one generated by MSVC and g++, they do not insert a call for the move constructor (but to be clear, I've found a comment in the corresponding code of clang what unsure me who does the correct thing in this situation).

The code:

#include <cstdio>

struct A {
  /*explicit */A(const char *) { printf("A(const char *)\n"); }
  A(const A &) { printf("A(const A &)\n"); }
  A(A&&) { printf("A(A&&)\n"); }
  A &operator=(const A &) { printf("operator=(const A&)\n"); return *this; }
  A &operator=(A &&) { printf("operator=(A&&)\n"); return *this; }
  ~A() { printf("~A()\n"); }
};

struct B : A {};

B rvo() {
  return B{{"Hi"}};
}

B norvo() {
  return B{A{"Hi2"}};
}

A rvoa() {
  return {"hi"};
}

A norvoa() {
  return A{"hi2"};
}

int main() {
  {
    printf("rvo:\n");
    B b = rvo();
    A a = rvoa();
  }

  {
    printf("no rvo:\n");
    B b = norvo();
    A a = norvoa();
  }

  return 0;
}

So, when an object of type B is created using B{{"Hi"}}, everything is fine and no move-constructor for A is invoked, but when the object of B is created using B{A{"Hi2"}}, the move constructor for A is invoked.

Analysis

The following AST was generated for the first case (B{{"Hi"}}):

|-FunctionDecl 0x28607286238 <line:14:1, line:16:1> line:14:3 used rvo 'B ()'
| `-CompoundStmt 0x28607286620 <col:9, line:16:1>
|   `-ReturnStmt 0x28607286610 <line:15:3, col:18>
|     `-ExprWithCleanups 0x286072865f8 <col:10, col:18> 'B':'B'
|       `-CXXFunctionalCastExpr 0x286072865d0 <col:10, col:18> 'B':'B' functional cast to B <NoOp>
|         `-CXXBindTemporaryExpr 0x286072865b0 <col:11, col:18> 'B':'B' (CXXTemporary 0x286072865b0)
|           `-InitListExpr 0x28607286418 <col:11, col:18> 'B':'B'
|             `-CXXConstructExpr 0x28607286568 <col:12, col:17> 'A':'A' 'void (const char *)' list
|               `-ImplicitCastExpr 0x28607286460 <col:13> 'const char *' <ArrayToPointerDecay>
|                 `-StringLiteral 0x28607286368 <col:13> 'const char[3]' lvalue "Hi"

while the AST for the second case (B{A{"Hi2"}}) contains a MaterializeTemporaryExpr expression:

|-FunctionDecl 0x28607286658 <line:18:1, line:20:1> line:18:3 used norvo 'B ()'
| `-CompoundStmt 0x286072869c8 <col:11, line:20:1>
|   `-ReturnStmt 0x286072869b8 <line:19:3, col:20>
|     `-ExprWithCleanups 0x286072869a0 <col:10, col:20> 'B':'B'
|       `-CXXFunctionalCastExpr 0x28607286978 <col:10, col:20> 'B':'B' functional cast to B <NoOp>
|         `-CXXBindTemporaryExpr 0x28607286958 <col:11, col:20> 'B':'B' (CXXTemporary 0x28607286958)
|           `-InitListExpr 0x286072868c0 <col:11, col:20> 'B':'B'
|             `-CXXConstructExpr 0x28607286920 <col:12, col:19> 'A':'A' 'void (A &&)'
|               `-MaterializeTemporaryExpr 0x28607286908 <col:12, col:19> 'A':'A' xvalue
|                 `-CXXBindTemporaryExpr 0x28607286858 <col:12, col:19> 'A':'A' (CXXTemporary 0x28607286858)
|                   `-CXXTemporaryObjectExpr 0x28607286818 <col:12, col:19> 'A':'A' 'void (const char *)' list
|                     `-ImplicitCastExpr 0x28607286800 <col:14> 'const char *' <ArrayToPointerDecay>
|                       `-StringLiteral 0x28607286798 <col:14> 'const char[4]' lvalue "Hi2"

After some debugging, I've found the following code in the SemaInit.cpp file, which is responsible for adding a QualificationConversion step into the sequence of initialization actions:

  // C++17 [dcl.init]p17:
  //     - If the initializer expression is a prvalue and the cv-unqualified
  //       version of the source type is the same class as the class of the
  //       destination, the initializer expression is used to initialize the
  //       destination object.
  // Per DR (no number yet), this does not apply when initializing a base
  // class or delegating to another constructor from a mem-initializer.
  // ObjC++: Lambda captured by the block in the lambda to block conversion
  // should avoid copy elision.
  if (S.getLangOpts().CPlusPlus17 && !RequireActualConstructor &&
      UnwrappedArgs.size() == 1 && UnwrappedArgs[0]->isPRValue() &&
      S.Context.hasSameUnqualifiedType(UnwrappedArgs[0]->getType(), DestType)) {
    // Convert qualifications if necessary.
    Sequence.AddQualificationConversionStep(DestType, VK_PRValue);
    ...
    return;
  }
  ...

But this code works only for the first case because only in this case the RequireActualConstructor variable is set to false. In the second case, with an explicit constructor call, the variable is set to true and no-early return occurs, the rest of the void TryConstructorInitialization() function is executed what ends to the adding a ConstructorInitialization step into the sequence of initialization actions:

  // Add the constructor initialization step. Any cv-qualification conversion is
  // subsumed by the initialization.
  Sequence.AddConstructorInitializationStep(
      Best->FoundDecl, CtorDecl, DestArrayType, HadMultipleCandidates,
      IsListInit | IsInitListCopy, AsInitializerList);

The RequireActualConstructor variable is set by the following expression:

bool RequireActualConstructor =
      !(Entity.getKind() != InitializedEntity::EK_Base &&
        Entity.getKind() != InitializedEntity::EK_Delegating &&
        Entity.getKind() !=
            InitializedEntity::EK_LambdaToBlockConversionBlockElement);

In this case, we have Entity.getKind() equals to InitializedEntity::EK_Base. When the decision for the RequireActualConstructor variable is changed (this comparison was excluded), the observed behavior is changed too and no calling of the move constructor is generated anymore.

This may look as a fix until we see the second part of the comment on line 4264:

  // Per DR (no number yet), this does not apply when initializing a base
  // class or delegating to another constructor from a mem-initializer.

I didn't manage to find the corresponding DR, as I see this is now (hm, now, I mean 7 years ago?) a part of the C++17 standard as well as all the subsequent versions and before changing anything in the code the question about legality of such change should be considered. But the difference in the behavior between clang and MSVC + gcc makes me wonder.