[clang] 2bd8493 - Improve type printing of const arrays to normalize array-of-const and const-array
David Blaikie via cfe-commits
cfe-commits at lists.llvm.org
Thu Oct 14 14:25:18 PDT 2021
On Tue, Oct 12, 2021 at 7:35 PM David Blaikie <dblaikie at gmail.com> wrote:
> On Mon, Oct 11, 2021 at 2:46 PM Richard Smith <richard at metafoo.co.uk>
> wrote:
>
>> On Wed, 15 Sept 2021 at 13:52, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>> On Tue, Sep 14, 2021 at 10:04 AM Richard Smith <richard at metafoo.co.uk>
>>> wrote:
>>>
>>>> On Mon, 13 Sept 2021 at 19:24, David Blaikie via cfe-commits <
>>>> cfe-commits at lists.llvm.org> wrote:
>>>>
>>>>>
>>>>> Author: David Blaikie
>>>>> Date: 2021-09-13T19:17:05-07:00
>>>>> New Revision: 2bd84938470bf2e337801faafb8a67710f46429d
>>>>>
>>>>> URL:
>>>>> https://github.com/llvm/llvm-project/commit/2bd84938470bf2e337801faafb8a67710f46429d
>>>>> DIFF:
>>>>> https://github.com/llvm/llvm-project/commit/2bd84938470bf2e337801faafb8a67710f46429d.diff
>>>>>
>>>>> LOG: Improve type printing of const arrays to normalize array-of-const
>>>>> and const-array
>>>>>
>>>>> Since these map to the same effective type - render them the same/in
>>>>> the
>>>>> more legible way (const x[n]).
>>>>>
>>>>
>>>> Nice!
>>>>
>>>>
>>>>> Added:
>>>>>
>>>>>
>>>>> Modified:
>>>>> clang/lib/AST/TypePrinter.cpp
>>>>> clang/test/ARCMT/cxx-checking.mm
>>>>> clang/test/AST/ast-dump-APValue-arithmetic.cpp
>>>>> clang/test/AST/ast-dump-APValue-array.cpp
>>>>> clang/test/CXX/basic/basic.types/p10.cpp
>>>>> clang/test/Sema/assign.c
>>>>> clang/test/Sema/typedef-retain.c
>>>>> clang/test/SemaCXX/reinterpret-cast.cpp
>>>>> clang/test/SemaCXX/static-assert-cxx17.cpp
>>>>>
>>>>> Removed:
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> ################################################################################
>>>>> diff --git a/clang/lib/AST/TypePrinter.cpp
>>>>> b/clang/lib/AST/TypePrinter.cpp
>>>>> index aef1e4f3f4953..251db97c7db08 100644
>>>>> --- a/clang/lib/AST/TypePrinter.cpp
>>>>> +++ b/clang/lib/AST/TypePrinter.cpp
>>>>> @@ -200,11 +200,12 @@ bool TypePrinter::canPrefixQualifiers(const Type
>>>>> *T,
>>>>> // type expands to a simple string.
>>>>> bool CanPrefixQualifiers = false;
>>>>> NeedARCStrongQualifier = false;
>>>>> - Type::TypeClass TC = T->getTypeClass();
>>>>> + const Type *UnderlyingType = T;
>>>>> if (const auto *AT = dyn_cast<AutoType>(T))
>>>>> - TC = AT->desugar()->getTypeClass();
>>>>> + UnderlyingType = AT->desugar().getTypePtr();
>>>>> if (const auto *Subst = dyn_cast<SubstTemplateTypeParmType>(T))
>>>>> - TC = Subst->getReplacementType()->getTypeClass();
>>>>> + UnderlyingType = Subst->getReplacementType().getTypePtr();
>>>>> + Type::TypeClass TC = UnderlyingType->getTypeClass();
>>>>>
>>>>> switch (TC) {
>>>>> case Type::Auto:
>>>>> @@ -243,6 +244,9 @@ bool TypePrinter::canPrefixQualifiers(const Type
>>>>> *T,
>>>>>
>>>>> case Type::ConstantArray:
>>>>> case Type::IncompleteArray:
>>>>> + return canPrefixQualifiers(
>>>>> +
>>>>> cast<ArrayType>(UnderlyingType)->getElementType().getTypePtr(),
>>>>> + NeedARCStrongQualifier);
>>>>> case Type::VariableArray:
>>>>> case Type::DependentSizedArray:
>>>>>
>>>>
>>>> Can we give these two cases the same treatment?
>>>>
>>>
>>> Handled the DependentSizedArray in
>>> https://github.com/llvm/llvm-project/commit/40acc0adad59ac39e9a7a02fcd93161298500c00
>>>
>>> But per the comment in that commit I wasn't able to reproduce the
>>> problem with a variable array - though we could include it in the handling
>>> on principle/for consistency, even without a test/etc. Perhaps there's a
>>> way to test/provoke the behavior you might know that I couldn't figure out?
>>>
>>> Details from the commit:
>>>
>>> The VariableArray case I couldn't figure out how to test/provoke -
>>> you
>>>
>>> can't write/form a variable array in any context other than a local
>>>
>>> variable that I know of, and in that case `const int x[n]` is the
>>>
>>> normalized form already (array-of-const) and you can't use typedefs
>>>
>>> (since you can't typedef int[n] with variable 'n') to force the
>>>
>>> const-array AST that would produce the undesirable type printing
>>> "int
>>>
>>> const [n]".
>>>
>>
>> C has a fairly surprising rule here -- you actually can define a VLA type
>> in a local typedef. For example,
>>
>> void f(int n) {
>> typedef int arr[n];
>> const arr x;
>> }
>>
>> ... is valid in C. In C++ mode, this produces:
>>
>> <source>:3:15: error: default initialization of an object of const type
>> 'const arr' (aka 'int const[n]')
>>
>> (note, 'int const[n]' not 'const int[n]').
>>
>
> Oh, right, good to understand! Yep, fixed and tested with this:
> 39093279f2ede4af9048b89d048d7fe9182a50f8
>
> Huh, funny diagnostic experience aside here:
> https://godbolt.org/z/W46b3qc49
> void f(int i) {
> typedef int x[i];
> const x y = {};
> }
> <source>:2:19: error: variable-sized object may not be initialized
> typedef int x[i];
> ^
>
> With no pointer/note/etc to the 'y' variable - the typedef and variable
> could be quite far apart. The diagnostic would be hard to read/understand.
>
>
>> Oh, also - another quirk of array type printing that I'd be up for
>>> addressing if you've got an opinion on it:
>>>
>>> "int [n]" is printed with a space between the "int" and array.
>>> "int *const[n]" is printed without a space before the array (though both
>>> have a space after "int")
>>>
>>> ClangFormat formats these as "int[n]" and "int *const[n]" - with *
>>> left/right depending on ClangFormat's heuristics/configuration.
>>>
>>> Reckon we should remove the space in "int[n]"?
>>>
>>
>> I think we should make it consistent, one way or the other. Beyond that,
>> the decision seems somewhat arbitrary, but following clang-format seems
>> reasonable to me. I certainly would expect `int *[n]` rather than `int *
>> [n]`, which similarly suggests omitting the space here.
>>
>
Hmm, coming back to this I might've mangled up my test cases. I can't seem
to reproduce the "int *const[n]" printing (without the space) - hmm, maybe
when there's a typedef or something forcing the const array rather than
array of const situation?
Ah, yep, there it is:
*test.cpp:1:6: note: *candidate function not viable: no known conversion
from 'int [3]' to 'int' for 1st argument
*test.cpp:1:6: note: *candidate function not viable: no known conversion
from 'int (*const)[4]' to 'int' for 1st argument
*test.cpp:1:6: note: *candidate function not viable: no known conversion
from 'int *const [4]' to 'int' for 1st argument
*test.cpp:1:6: note: *candidate function not viable: no known conversion
from 'const int_star_arr' (aka 'int *const[4]') to 'int' for 1st argument
And, yeah, removing the NonEmptyPH's from the array type printers gets to a
consistent result:
*test.cpp:1:6: note: *candidate function not viable: no known conversion
from 'int[3]' to 'int' for 1st argument
*test.cpp:1:6: note: *candidate function not viable: no known conversion
from 'int (*const)[4]' to 'int' for 1st argument
*test.cpp:1:6: note: *candidate function not viable: no known conversion
from 'int *const[4]' to 'int' for 1st argument
*test.cpp:1:6: note: *candidate function not viable: no known conversion
from 'const int_star_arr' (aka 'int *const[4]') to 'int' for 1st argument
and I was going to say these are all consistent with clang-format, but the
second ("int (*const)[4]") isn't. Clang-format renders that as
"int(*const)[4]", even with a named variable of that type it still goes
with "int(*const y)[4]". That doesn't seem like the most common
formatting/surprises me a bit I & I can't see a consistent rule that would
produce that formatting there, compared to including the space in "int
*const[4]". But maybe that's adequately out of scope for this array
situation.
It looks like the cause is this:
> https://github.com/llvm/llvm-project/blob/40acc0adad59ac39e9a7a02fcd93161298500c00/clang/lib/AST/TypePrinter.cpp#L508
>
> But there seem to be a lot of similar code with NonEmptyPH that presumably
> has similar effects - any idea why?
> Cases include:
>
> PointerBefore (changes 'char *' to 'char*' (though 'char *[N]' keeps the
> space before the '*')
> PointerAfter (ah, seems to be related to the closing ")" in "void
> (*const)" - so I would've /thought/ the leading "(" would be disabled by
> removing the NonEmptyPH in PointerBefore... but apparently not? So I'm not
> sure why the asymmetry here - might be more related to the PrevPHIsEmpty in
> printBefore(Type*, Qualifiers, raw_ostream))
> BlockPointerBefore
> BlockPointerAfter
> LValueReferenceBefore
> LValueReferenceAfter
> RValueReferenceBefore
> RValueReferenceAfter
> MemberPointerBefore
> MemberPointerAfter
> ConstantArrayBefore
> IncompleteArrayBefore
> VariableArrayBefore
> DependentSizedArrayBefore
> FunctionProtoAfter
> FunctionNoProtoAfter
>
> Still playing around with some other cases here to better understand. I
> guess some stuff uses NonEmptyPlaceHolder for when, for instance, the
> qualifier has to go on the right ("void *const") so when printing the
> underlying pointer type, it has a non-empty place holder (the const) but
> the const itself has an empty placeholder (no variable name), etc.
>
Hmm, actually I think I see the logic - for all the pointer and reference
cases, the pointer/reference token goes to the right of the left side of
the type printing - so for any nested type printing, its as
though/equivalent to having a non-empty placeholder (because there will be
a token to their right) even if there is no actual/underlying placeholder.
So I think I'm pretty happy with just addressing the array cases here. Made
the array change here: 277623f4d5a672d707390e2c3eaf30a9eb4b075c
>
> I'll try just removing the array ones, see what that looks like.
>
> NeedARCStrongQualifier = true;
>>>>
>>>>
>>>>> diff --git a/clang/test/ARCMT/cxx-checking.mm b/clang/test/ARCMT/
>>>>> cxx-checking.mm
>>>>> index 952f3cdcbb79c..d6441def09b40 100644
>>>>> --- a/clang/test/ARCMT/cxx-checking.mm
>>>>> +++ b/clang/test/ARCMT/cxx-checking.mm
>>>>> @@ -80,7 +80,7 @@
>>>>>
>>>>> struct FlexibleArrayMember0 {
>>>>> int length;
>>>>> - id array[]; // expected-error{{flexible array member 'array' of
>>>>> type 'id __strong[]' with non-trivial destruction}}
>>>>> + id array[]; // expected-error{{flexible array member 'array' of
>>>>> type '__strong id []' with non-trivial destruction}}
>>>>> };
>>>>>
>>>>> struct FlexibleArrayMember1 {
>>>>>
>>>>> diff --git a/clang/test/AST/ast-dump-APValue-arithmetic.cpp
>>>>> b/clang/test/AST/ast-dump-APValue-arithmetic.cpp
>>>>> index 835d8c8108346..e51c1cee04cfe 100644
>>>>> --- a/clang/test/AST/ast-dump-APValue-arithmetic.cpp
>>>>> +++ b/clang/test/AST/ast-dump-APValue-arithmetic.cpp
>>>>> @@ -36,13 +36,13 @@ void Test() {
>>>>> // CHECK-NEXT: | |-value: ComplexFloat 3.141500e+00 +
>>>>> 4.200000e+01i
>>>>>
>>>>> constexpr _Complex int ArrayOfComplexInt[10] = {ComplexInt,
>>>>> ComplexInt, ComplexInt, ComplexInt};
>>>>> - // CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}}
>>>>> ArrayOfComplexInt '_Complex int const[10]' constexpr cinit
>>>>> + // CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}}
>>>>> ArrayOfComplexInt 'const _Complex int [10]' constexpr cinit
>>>>> // CHECK-NEXT: | |-value: Array size=10
>>>>> // CHECK-NEXT: | | |-elements: ComplexInt 42 + 24i, ComplexInt
>>>>> 42 + 24i, ComplexInt 42 + 24i, ComplexInt 42 + 24i
>>>>> // CHECK-NEXT: | | `-filler: 6 x ComplexInt 0 + 0i
>>>>>
>>>>> constexpr _Complex float ArrayOfComplexFloat[10] = {ComplexFloat,
>>>>> ComplexFloat, ComplexInt, ComplexInt};
>>>>> - // CHECK: `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}}
>>>>> ArrayOfComplexFloat '_Complex float const[10]' constexpr cinit
>>>>> + // CHECK: `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}}
>>>>> ArrayOfComplexFloat 'const _Complex float [10]' constexpr cinit
>>>>> // CHECK-NEXT: |-value: Array size=10
>>>>> // CHECK-NEXT: | |-elements: ComplexFloat 3.141500e+00 +
>>>>> 4.200000e+01i, ComplexFloat 3.141500e+00 + 4.200000e+01i, ComplexFloat
>>>>> 4.200000e+01 + 2.400000e+01i, ComplexFloat 4.200000e+01 + 2.400000e+01i
>>>>> // CHECK-NEXT: | `-filler: 6 x ComplexFloat 0.000000e+00 +
>>>>> 0.000000e+00i
>>>>>
>>>>> diff --git a/clang/test/AST/ast-dump-APValue-array.cpp
>>>>> b/clang/test/AST/ast-dump-APValue-array.cpp
>>>>> index f9b38ec332a5b..72e519208ac56 100644
>>>>> --- a/clang/test/AST/ast-dump-APValue-array.cpp
>>>>> +++ b/clang/test/AST/ast-dump-APValue-array.cpp
>>>>> @@ -43,7 +43,7 @@ void Test() {
>>>>> constexpr float arr_f[3][5] = {
>>>>> {1, 2, 3, 4, 5},
>>>>> };
>>>>> - // CHECK: | `-VarDecl {{.*}} <line:{{.*}}, line:{{.*}}>
>>>>> line:{{.*}} arr_f 'float const[3][5]' constexpr cinit
>>>>> + // CHECK: | `-VarDecl {{.*}} <line:{{.*}}, line:{{.*}}>
>>>>> line:{{.*}} arr_f 'const float [3][5]' constexpr cinit
>>>>> // CHECK-NEXT: | |-value: Array size=3
>>>>> // CHECK-NEXT: | | |-element: Array size=5
>>>>> // CHECK-NEXT: | | | |-elements: Float 1.000000e+00, Float
>>>>> 2.000000e+00, Float 3.000000e+00, Float 4.000000e+00
>>>>> @@ -52,7 +52,7 @@ void Test() {
>>>>> // CHECK-NEXT: | | `-filler: 5 x Float 0.000000e+00
>>>>>
>>>>> constexpr S0 arr_s0[2] = {{1, 2}, {3, 4}};
>>>>> - // CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}}
>>>>> arr_s0 'S0 const[2]' constexpr cinit
>>>>> + // CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}}
>>>>> arr_s0 'const S0 [2]' constexpr cinit
>>>>> // CHECK-NEXT: | |-value: Array size=2
>>>>> // CHECK-NEXT: | | |-element: Struct
>>>>> // CHECK-NEXT: | | | `-field: Array size=2
>>>>> @@ -62,12 +62,12 @@ void Test() {
>>>>> // CHECK-NEXT: | | `-elements: Int 3, Int 4
>>>>>
>>>>> constexpr U0 arr_u0[2] = {{.i = 42}, {.f = 3.1415f}};
>>>>> - // CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}}
>>>>> arr_u0 'U0 const[2]' constexpr cinit
>>>>> + // CHECK: | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}}
>>>>> arr_u0 'const U0 [2]' constexpr cinit
>>>>> // CHECK-NEXT: | |-value: Array size=2
>>>>> // CHECK-NEXT: | | `-elements: Union .i Int 42, Union .f Float
>>>>> 3.141500e+00
>>>>>
>>>>> constexpr S1 arr_s1[2] = {};
>>>>> - // CHECK: `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}}
>>>>> arr_s1 'S1 const[2]' constexpr cinit
>>>>> + // CHECK: `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}}
>>>>> arr_s1 'const S1 [2]' constexpr cinit
>>>>> // CHECK-NEXT: |-value: Array size=2
>>>>> // CHECK-NEXT: | |-element: Struct
>>>>> // CHECK-NEXT: | | |-field: Struct
>>>>>
>>>>> diff --git a/clang/test/CXX/basic/basic.types/p10.cpp
>>>>> b/clang/test/CXX/basic/basic.types/p10.cpp
>>>>> index aac07c76bed2e..eae0c6513ac24 100644
>>>>> --- a/clang/test/CXX/basic/basic.types/p10.cpp
>>>>> +++ b/clang/test/CXX/basic/basic.types/p10.cpp
>>>>> @@ -32,7 +32,7 @@ struct Incomplete; // expected-note 2{{forward
>>>>> declaration of 'Incomplete'}}
>>>>> template<class T> struct ClassTemp {};
>>>>>
>>>>> constexpr Incomplete incomplete = {}; // expected-error {{constexpr
>>>>> variable cannot have non-literal type 'const Incomplete'}} expected-note
>>>>> {{incomplete type 'const Incomplete' is not a literal type}}
>>>>> -constexpr Incomplete incomplete2[] = {}; // expected-error
>>>>> {{constexpr variable cannot have non-literal type 'Incomplete const[]'}}
>>>>> expected-note {{incomplete type 'Incomplete const[]' is not a literal type}}
>>>>> +constexpr Incomplete incomplete2[] = {}; // expected-error
>>>>> {{constexpr variable cannot have non-literal type 'const Incomplete []'}}
>>>>> expected-note {{incomplete type 'const Incomplete []' is not a literal
>>>>> type}}
>>>>> constexpr ClassTemp<int> classtemplate = {};
>>>>> constexpr ClassTemp<int> classtemplate2[] = {};
>>>>>
>>>>>
>>>>> diff --git a/clang/test/Sema/assign.c b/clang/test/Sema/assign.c
>>>>> index 3edd5d4feaf2e..d65f9bf26547e 100644
>>>>> --- a/clang/test/Sema/assign.c
>>>>> +++ b/clang/test/Sema/assign.c
>>>>> @@ -13,7 +13,7 @@ typedef int arr[10];
>>>>> void test3() {
>>>>> const arr b; // expected-note {{variable 'b' declared const
>>>>> here}}
>>>>> const int b2[10]; // expected-note {{variable 'b2' declared const
>>>>> here}}
>>>>> - b[4] = 1; // expected-error {{cannot assign to variable 'b'
>>>>> with const-qualified type 'const arr' (aka 'int const[10]')}}
>>>>> + b[4] = 1; // expected-error {{cannot assign to variable 'b'
>>>>> with const-qualified type 'const arr' (aka 'const int [10]')}}
>>>>> b2[4] = 1; // expected-error {{cannot assign to variable
>>>>> 'b2' with const-qualified type 'const int [10]'}}
>>>>> }
>>>>>
>>>>>
>>>>> diff --git a/clang/test/Sema/typedef-retain.c
>>>>> b/clang/test/Sema/typedef-retain.c
>>>>> index 70e2abc1da5b8..d4358bce5ee5b 100644
>>>>> --- a/clang/test/Sema/typedef-retain.c
>>>>> +++ b/clang/test/Sema/typedef-retain.c
>>>>> @@ -17,7 +17,7 @@ typedef int a[5];
>>>>> void test3() {
>>>>> typedef const a b;
>>>>> b r; // expected-note {{variable 'r' declared const here}}
>>>>> - r[0] = 10; // expected-error {{cannot assign to variable 'r' with
>>>>> const-qualified type 'b' (aka 'int const[5]')}}
>>>>> + r[0] = 10; // expected-error {{cannot assign to variable 'r' with
>>>>> const-qualified type 'b' (aka 'const int [5]')}}
>>>>> }
>>>>>
>>>>> int test4(const a y) {
>>>>>
>>>>> diff --git a/clang/test/SemaCXX/reinterpret-cast.cpp
>>>>> b/clang/test/SemaCXX/reinterpret-cast.cpp
>>>>> index 6a4bc91665448..f427af1efbafc 100644
>>>>> --- a/clang/test/SemaCXX/reinterpret-cast.cpp
>>>>> +++ b/clang/test/SemaCXX/reinterpret-cast.cpp
>>>>> @@ -132,7 +132,7 @@ void const_arrays() {
>>>>> const STRING *s;
>>>>> const char *c;
>>>>>
>>>>> - (void)reinterpret_cast<char *>(s); // expected-error
>>>>> {{reinterpret_cast from 'const STRING *' (aka 'char const (*)[10]') to
>>>>> 'char *' casts away qualifiers}}
>>>>> + (void)reinterpret_cast<char *>(s); // expected-error
>>>>> {{reinterpret_cast from 'const STRING *' (aka 'const char (*)[10]') to
>>>>> 'char *' casts away qualifiers}}
>>>>> (void)reinterpret_cast<const STRING *>(c);
>>>>> }
>>>>>
>>>>>
>>>>> diff --git a/clang/test/SemaCXX/static-assert-cxx17.cpp
>>>>> b/clang/test/SemaCXX/static-assert-cxx17.cpp
>>>>> index fe02f8f29965f..d5601839bdd3e 100644
>>>>> --- a/clang/test/SemaCXX/static-assert-cxx17.cpp
>>>>> +++ b/clang/test/SemaCXX/static-assert-cxx17.cpp
>>>>> @@ -94,7 +94,7 @@ void foo6() {
>>>>> static_assert(static_cast<const X<typename T::T> *>(nullptr));
>>>>> // expected-error at -1{{static_assert failed due to requirement
>>>>> 'static_cast<const X<int> *>(nullptr)'}}
>>>>> static_assert((const X<typename T::T>[]){} == nullptr);
>>>>> - // expected-error at -1{{static_assert failed due to requirement
>>>>> '(X<int> const[0]){} == nullptr'}}
>>>>> + // expected-error at -1{{static_assert failed due to requirement
>>>>> '(const X<int> [0]){} == nullptr'}}
>>>>> static_assert(sizeof(X<decltype(X<typename T::T>().X<typename
>>>>> T::T>::~X())>) == 0);
>>>>> // expected-error at -1{{static_assert failed due to requirement
>>>>> 'sizeof(X<void>) == 0'}}
>>>>> static_assert(constexpr_return_false<typename T::T, typename
>>>>> T::U>());
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> cfe-commits mailing list
>>>>> cfe-commits at lists.llvm.org
>>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>
>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20211014/a450c42d/attachment-0001.html>
More information about the cfe-commits
mailing list