On Tue, Oct 12, 2021 at 7:35 PM David Blaikie <dblai...@gmail.com> wrote:
> On Mon, Oct 11, 2021 at 2:46 PM Richard Smith <rich...@metafoo.co.uk> > wrote: > >> On Wed, 15 Sept 2021 at 13:52, David Blaikie <dblai...@gmail.com> wrote: >> >>> On Tue, Sep 14, 2021 at 10:04 AM Richard Smith <rich...@metafoo.co.uk> >>> wrote: >>> >>>> On Mon, 13 Sept 2021 at 19:24, David Blaikie via cfe-commits < >>>> cfe-commits@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@-1{{static_assert failed due to requirement >>>>> 'static_cast<const X<int> *>(nullptr)'}} >>>>> static_assert((const X<typename T::T>[]){} == nullptr); >>>>> - // expected-error@-1{{static_assert failed due to requirement >>>>> '(X<int> const[0]){} == nullptr'}} >>>>> + // expected-error@-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@-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@lists.llvm.org >>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>>>> >>>>
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
