Fix type printing of array template args

[reikdas]

Summary

constexpr const char kEta[] = "Eta";
template <const char*, typename T> class Column {};
using quick = Column<kEta,double>;

void lookup() {
   quick c1;
   c1.ls();
}

emits error: no member named 'ls' in 'Column<&kEta, double>' Attached patch fixes the printed type name by not printing the ampersand for array types.

It passes check-clang for me.

If this change is acceptable: where do you want a test to go?

[rjmccall]

I think test/SemaTemplate/temp_arg_nontype.cpp is probably the most appropriate place.

Unfortunately, I don't think the fix is totally correct. Both &kEta and kEta are valid template arguments, just for template parameters of different types. You need to check getParamTypeForDecl(), and, unfortunately, the possibility of qualification conversions will make the check somewhat annoying.

On the plus side, if you're checking the type, you can also resolve that FIXME about reference arguments. I think this code probably predates the inclusion of a parameter type in Decl TemplateArguments.

[karies]

Only write out an ampersand if needed to make parameter and argument match types.
Increase test coverage (including a text suggested by @rjmccall using an array param) and adjust existing tests' expected diagnostics to follow this change.

[karies]

...and remove FIXME.

[rjmccall]

Unfortunately, qualification conversions can be nested, e.g. a char** can be converted to a const char * const *. I think it would probably be easiest to just count the nesting depths of arrays.

[v.g.vassilev]

ping...

[reikdas]

Addresses @rjmccall 's comment

[v.g.vassilev]

ping...

[rjmccall]

Sorry, this got lost.

clang/include/clang/AST/Type.h

1895 ↗

(On Diff #302234)

I don't think this makes any sense to add as a general routine. If we do add it, though, it should be getInnermostPointerType(); "innermost" is a single word.

clang/lib/AST/TemplateBase.cpp

380 ↗

(On Diff #302234)

Please make a helper function for deciding whether an ampersand is required.

There's a lot more going on in this code than needs to be there; I don't know what this string comparison is supposed to accomplish, for example. It's okay to assume that the argument is well-formed for the parameter, which means you really do just need to compare pointer depths, like:

static unsigned getPointerDepth(QualType type) {
  unsigned count;
  while (auto ptrType = type->getAs<PointerType>()) {
    count++;
    type = ptrType->getPointeeType();
  }
  return count;
}

static bool needsAmpersandOnTemplateArg(QualType paramType, QualType argType) {
  if (paramType->isReferenceType()) return false;
  if (argType->isArrayType()) {
    // Decide whether we've decayed the array (to get a pointer to the element type)
    // or taken its address (to get a pointer to the array type).  We have to ignore
    // qualifiers here at arbitrary levels because of qualification conversions.
    return (getPointerDepth(paramType) >
            getPointerDepth(QualType(argType->getArrayElementTypeNoTypeQual(), 0)));
  }
  return true;
}

clang/test/SemaTemplate/temp_arg_nontype_cxx11.cpp

89 ↗

(On Diff #302234)

Please add both positive and negative tests (i.e. both array decays and otherwise) for each of these.

[reikdas]

Address comments.

[rjmccall]

clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp

87 ↗

(On Diff #340736)

Why did this change? This is a decay and should probably continue to be printed with &.

[reikdas]

Address @rjmccall inline comment.

[rjmccall]

clang/lib/AST/TemplateBase.cpp

109 ↗

(On Diff #341424)

Hmm. This seems unnecessarily complex. Oh, and I think I suggested the comparison wrong for pointer depth.

Is there a case where the following doesn't work?

static unsigned getArrayDepth(QualType type) {
  unsigned count = 0;
  while (const auto *arrayType = type->getAsArrayTypeUnsafe()) {
    count++;
    type = arrayType->getElementType();
  }
  return count;
}

static bool needsAmpersandOnTemplateArg(QualType paramType, QualType argType,
                                        ASTContext &Ctx) {
  if (!paramType->isPointerType())
    return false;
  if (argType->isFunctionType())
    return true;
  if (argType->isArrayType())
    return getArrayDepth(argType) < getArrayDepth(paramType->getPointeeType());
  return false;
}

[reikdas]

clang/lib/AST/TemplateBase.cpp

109 ↗

(On Diff #341424)

Many tests fail with this.

For eg., the tests added to SemaTemplate/temp_arg_nontype_cxx11.cpp

********************
FAIL: Clang :: SemaTemplate/temp_arg_nontype_cxx11.cpp (13255 of 27894)
******************** TEST 'Clang :: SemaTemplate/temp_arg_nontype_cxx11.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';   /home/reik/llvm-project/build/bin/clang -cc1 -internal-isystem /home/reik/llvm-project/build/lib/clang/13.0.0/include -nostdsysteminc -fsyntax-only -verify -std=c++11 /home/reik/llvm-project/clang/test/SemaTemplate/temp_arg_nontype_cxx11.cpp
--
Exit Code: 1

Command Output (stderr):
--
error: 'error' diagnostics expected but not seen: 
  File /home/reik/llvm-project/clang/test/SemaTemplate/temp_arg_nontype_cxx11.cpp Line 102: <&kEta,
  File /home/reik/llvm-project/clang/test/SemaTemplate/temp_arg_nontype_cxx11.cpp Line 103: <&kDes,
  File /home/reik/llvm-project/clang/test/SemaTemplate/temp_arg_nontype_cxx11.cpp Line 104: <&kEta,
  File /home/reik/llvm-project/clang/test/SemaTemplate/temp_arg_nontype_cxx11.cpp Line 105: <&kDes,
  File /home/reik/llvm-project/clang/test/SemaTemplate/temp_arg_nontype_cxx11.cpp Line 106: <&kNull,
  File /home/reik/llvm-project/clang/test/SemaTemplate/temp_arg_nontype_cxx11.cpp Line 108: <&kZero,
error: 'error' diagnostics seen but not expected: 
  File /home/reik/llvm-project/clang/test/SemaTemplate/temp_arg_nontype_cxx11.cpp Line 102: no member named 'ls' in 'Folumn<kEta, double>'
  File /home/reik/llvm-project/clang/test/SemaTemplate/temp_arg_nontype_cxx11.cpp Line 103: no member named 'ls' in 'Folumn<kDes, double>'
  File /home/reik/llvm-project/clang/test/SemaTemplate/temp_arg_nontype_cxx11.cpp Line 104: no member named 'ls' in 'Golumn<kEta, double>'
  File /home/reik/llvm-project/clang/test/SemaTemplate/temp_arg_nontype_cxx11.cpp Line 105: no member named 'ls' in 'Golumn<kDes, double>'
  File /home/reik/llvm-project/clang/test/SemaTemplate/temp_arg_nontype_cxx11.cpp Line 106: no member named 'ls' in 'Holumn<kNull, double>'
  File /home/reik/llvm-project/clang/test/SemaTemplate/temp_arg_nontype_cxx11.cpp Line 108: no member named 'ls' in 'Iolumn<kZero, double>'
12 errors generated.

--

[rjmccall]

clang/lib/AST/TemplateBase.cpp

109 ↗

(On Diff #341424)

Oh yes, sorry, my logic was quite wrong. Try this:

static bool needsAmpersandOnTemplateArg(QualType paramType, QualType argType,
                                        ASTContext &Ctx) {
  // Generally, if the parameter type is a pointer, we must be taking the address of
  // something and need a &.  However, if the argument is an array, this could be
  // implicit via array-to-pointer decay.
  if (!paramType->isPointerType())
    return false;
  if (argType->isArrayType())
    return getArrayDepth(argType) == getArrayDepth(paramType->getPointeeType());
  return true;
}

[reikdas]

Address @rjmccall inline comment.

[rjmccall]

clang/test/CodeGenCXX/debug-info-template.cpp

3 ↗

(On Diff #341775)

Oh, this is wrong. needsAmpersandOnTemplateArg needs one other tweak:

if (!paramType->isPointerType())
   return paramType->is<MemberPointerType>();

Please also add tests for the member-pointer cases to temp_arg_nontype_cxx11.cpp.

[reikdas]

Address @rjmccall 's inline comment.

[rjmccall]

Thanks, LGTM

lib/AST/TemplateBase.cpp

case Type: {
     PrintingPolicy SubPolicy(Policy);
     SubPolicy.SuppressStrongLifetime = true;
     getAsType().print(Out, SubPolicy);
     break;
   }
     
   case Declaration: {
     NamedDecl *ND = cast<NamedDecl>(getAsDecl());
+    bool needsRef = true;
+    if (auto VD = dyn_cast<ValueDecl>(ND)) {
+      const clang::Type *ArgTy = VD->getType()->getUnqualifiedDesugaredType();
+      const clang::Type *ParmTy
+        = getParamTypeForDecl()->getUnqualifiedDesugaredType();
+      clang::ASTContext& Ctx = ND->getASTContext();
+      needsRef = !Ctx.hasSameType(ArgTy, ParmTy);
+      if (needsRef && (ArgTy->isArrayType() || ArgTy->isFunctionType())) {
+        const clang::Type *decayedArgTy
+          = Ctx.getDecayedType(clang::QualType(ArgTy, 0)).getTypePtr();
+        needsRef = !Ctx.hasSameType(decayedArgTy, ParmTy);
+      }
+    }
+    if (needsRef)
       Out << '&';
     if (ND->getDeclName()) {
-      // FIXME: distinguish between pointer and reference args?
       ND->printQualifiedName(Out);
     } else {
       Out << "(anonymous)";
     }
     break;
   }
 
   case NullPtr:

test/CodeGenCXX/debug-info-codeview-display-name.cpp

 // QUAL-DAG: "OverloadedNewDelete::operator delete[]"
 // QUAL-DAG: "OverloadedNewDelete::operator+"
 
 
 template <typename T, void (*)(void)>
 void fn_tmpl() {}
 
 template void fn_tmpl<int, freefunc>();
-// CHECK-DAG: "fn_tmpl<int,&freefunc>"
+// CHECK-DAG: "fn_tmpl<int,freefunc>"

test/CodeGenCXX/debug-info-template.cpp

 // RUN: %clang -S -emit-llvm -target x86_64-unknown_unknown -g %s -o - -std=c++11 | FileCheck %s
 
-// CHECK: @tci = global %"struct.TC<unsigned int, 2, &glb, &foo::e, &foo::f, &foo::g, 1, 2, 3>::nested" zeroinitializer, align 1, !dbg [[TCI:![0-9]+]]
+// CHECK: @tci = global %"struct.TC<unsigned int, 2, &glb, &foo::e, &foo::f, foo::g, 1, 2, 3>::nested" zeroinitializer, align 1, !dbg [[TCI:![0-9]+]]
 // CHECK: @tcn = global %struct.TC zeroinitializer, align 1, !dbg [[TCN:![0-9]+]]
 // CHECK: @nn = global %struct.NN zeroinitializer, align 1, !dbg [[NN:![0-9]+]]
 
 // CHECK: !DICompileUnit(
 // CHECK: [[EMPTY:![0-9]*]] = !{}
 
 struct foo {
   char pad[8]; // make the member pointer to 'e' a bit more interesting (nonzero)

 void func();
 
 // CHECK: [[TCI]] = !DIGlobalVariableExpression(var: [[TCIV:.*]])
 // CHECK: [[TCIV]] = distinct !DIGlobalVariable(name: "tci",
 // CHECK-SAME:                                  type: ![[TCNESTED:[0-9]+]]
 // CHECK: ![[TCNESTED]] ={{.*}}!DICompositeType(tag: DW_TAG_structure_type, name: "nested",
 // CHECK-SAME:             scope: ![[TC:[0-9]+]],
 
-// CHECK: ![[TC]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "TC<unsigned int, 2, &glb, &foo::e, &foo::f, &foo::g, 1, 2, 3>"
+// CHECK: ![[TC]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "TC<unsigned int, 2, &glb, &foo::e, &foo::f, foo::g, 1, 2, 3>"
 // CHECK-SAME:                              templateParams: [[TCARGS:![0-9]*]]
 TC
 // CHECK: [[TCARGS]] = !{[[TCARG1:![0-9]*]], [[TCARG2:![0-9]*]], [[TCARG3:![0-9]*]], [[TCARG4:![0-9]*]], [[TCARG5:![0-9]*]], [[TCARG6:![0-9]*]], [[TCARG7:![0-9]*]]}
 // CHECK: [[TCARG1]] = !DITemplateTypeParameter(name: "T", type: [[UINT:![0-9]*]])
 // CHECK: [[UINT:![0-9]*]] = !DIBasicType(name: "unsigned int"
 < unsigned,
 // CHECK: [[TCARG2]] = !DITemplateValueParameter(type: [[UINT]], value: i32 2)
   2,

test/SemaTemplate/temp_arg_nontype_cxx11.cpp

 namespace check_conversion_early {
   struct X {};
   template<int> struct A {};
   template<X &x> struct A<x> {}; // expected-error {{not implicitly convertible}}
 
   struct Y { constexpr operator int() const { return 0; } };
   template<Y &y> struct A<y> {}; // expected-error {{cannot be deduced}} expected-note {{'y'}}
 }
+
+using FourChars = const char[4];
+constexpr FourChars kEta = "Eta";
+constexpr const char* kNull = "Phi";
+
+template <const char*, typename T> class Column {};
+template <const char[], typename T> class Dolumn {};
+template <const char(*)[4], typename T> class Folumn {};
+template <FourChars*, typename T> class Golumn {};
+template <const char* const *, typename T> class Holumn {};
+
+void lookup() {
+  Column<kEta,double>().ls(); // expected-error {{<kEta,}}
+  Column<nullptr, double>().ls(); // expected-error {{<nullptr,}}
+  Dolumn<kEta, double>().ls(); // expected-error {{<kEta,}}
+  Folumn<&kEta, double>().ls(); // expected-error {{<&kEta,}}
+  Golumn<&kEta, double>().ls(); // expected-error {{<&kEta,}}
+  Holumn<&kNull, double>().ls(); // expected-error {{<&kNull,}}
+}