Patch for issue #22924

[Alexey Tarasov via cfe-commits]

This patch address issue 22924.

Context of the issue:
----------------------
Currently code

$ cat bad-init.cpp
class A {
public:
   A(int a): i_(a) {}
   int i_ = 0;
};

void foo() {
     A *p = new A[2] { A(3), A(5) };
}

is rejected with error

bad-init.cpp:8:36: error: no matching constructor for initialization of 'A'
     A *p = new A[2] { A(3), A(5) };


Some arguments in defence of this behaviour is that standard is not
completely clear in [expr.new] p19. In fact I was defending it as well,
when we discussed this case in our local cpp community. But there was a
disagreement on this mater, also it was pointed out that clang ignores
array size in new[] (see below), thus it allows producing ill-formed code
for aggregate initialisation.

Keeping this behaviour intact produces inconsistency because:
1) Ill-formed code shouldn't be accepted by compiler.
2) A *p = new A { A(1) }; // compiles fine in clang.
3) It differs from array initialisation: A a[2] { {1}, {2} }; // compiles
fine.
4) Other mainline compilers treat this code as valid.
5) clang backend itself produces code not using default constructor in the
following example:

$ cat good-init.cpp
class A {
public:
   A() {}
   A(int a): i_(a) {}
   int i_;
};

void foo() {
     A *p = new A[2] { A(3), A(5) };
}

$ clang++ -cc1 --std=c++11 -emit-llvm ./good-init.cpp; cat ./good-init.ll |
grep "foo" -A 11
define void @_Z3foov() #0 {
 %p = alloca %class.A*, align 8
 %1 = call noalias i8* @_Znam(i64 8) #2
 %2 = bitcast i8* %1 to %class.A*
 call void @_ZN1AC1Ei(%class.A* %2, i32 3)
 %3 = getelementptr inbounds %class.A, %class.A* %2, i64 1
 call void @_ZN1AC1Ei(%class.A* %3, i32 5)
 %4 = getelementptr inbounds %class.A, %class.A* %3, i64 1
 store %class.A* %2, %class.A** %p, align 8
 ret void
}

It seems that rejection of code written in bad-init.cpp is not caused by
interpretation of standard at all.
Roots of the issue are in a way Sema::BuildCXXNew() handles initialiser
list for arrays.

It is possible in clang to compile this code without any errors:

int *p = new int[2] { 1, 2, 3 };

Moreover, AST produced by parsing

int *p = new int[2] { 1, 2 };

reveals interesting fact:

|-VarDecl 0x10387e600 <./test.cpp:1:1, col:28> col:6 p 'int *' cinit
| `-CXXNewExpr 0x10387ee00 <col:10, col:28> 'int *' array Function
0x10387e8f0 'operator new[]' 'void *(unsigned long)'
|   |-IntegerLiteral 0x10387e6a0 <col:18> 'int' 2
|   `-InitListExpr 0x10387ed98 <col:21, col:28> 'int [3]'
|     |-array filler
|     | `-ImplicitValueInitExpr 0x10387edf0 <<invalid sloc>> 'int'
|     |-IntegerLiteral 0x10387e6c0 <col:23> 'int' 1
|     `-IntegerLiteral 0x10387e6e0 <col:26> 'int' 2

It treats int[2] as int[3] (note: 3 = 1 + number of elements in initialiser
lists). As result array filler appears out of nowhere. Emitted code
contains 3 (three) constructor calls, although those objects have no chance
to be constructed because new is called with all-ones value, thus with
libcxx on my machine it produces std::bad_alloc in runtime (although I’d
expect std::bad_array_new_length).

In brief, clang fails to compile code written in bad-init.cpp because
default constructor used as array filler initialiser is absent. Array
filler shouldn’t be used if number of initialisers is equal to a number of
array elements.

Attached file 22924.patch addresses this by changing way of handling arrays
with size specified by integer constant expressions.

The changes are:
* no array fillers are generated for a code similar to mentioned above.
* error is reported if number of initialisers for an array is bigger than a
number of that array elements.
Some notable differences in compiler diagnostic output are posted in
http://pastebin.com/2cNHf1kq

Patch passes all clang-test regression tests except:
1) CodeGenCXX/new-array-init.cpp, because it contains excess number of
initialisers which is treated as error after applying this patch:
CodeGenCXX/new-array-init.cpp:18:3: error: excess elements in array 'new'
initialiser
 new int[2] { 1, 2, 3 };

I believe this error report is correct.
That means test case const_underflow() should be removed, emitting code for
underflow cases makes sense only if size is not constant expression known
at compile-time.

2) SemaCXX/new-delete-cxx0x.cpp, because it explicitly rejects code which
is valid after applying patch. Basically this test contains simplified
version of the code sample from bad-init.cpp.

Therefore, I’ve updated those tests as well (see attached
22924-tests.patch).

—
Best regards,
Alexey Tarasov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160925/01d14221/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 22924-tests.patch
Type: application/octet-stream
Size: 1595 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160925/01d14221/attachment-0002.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 22924.patch
Type: application/octet-stream
Size: 6210 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160925/01d14221/attachment-0003.obj>