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

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

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

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

101

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

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

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

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

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

clang/include/clang/AST/Type.h

public:
   bool isSizelessType() const;
   bool isSizelessBuiltinType() const;
 
   /// Determines if this is a sizeless type supported by the
   /// 'arm_sve_vector_bits' type attribute, which can be applied to a single
   /// SVE vector or predicate, excluding tuple types such as svint32x4_t.
   bool isVLSTBuiltinType() const;
 
+  /// Returns the inner most Pointer Type node.
+  const PointerType *getInnerMostPointerType() const;
+
   /// Returns the representative type for the element of an SVE builtin type.
   /// This is used to represent fixed-length SVE vectors created with the
   /// 'arm_sve_vector_bits' type attribute as VectorType.
   QualType getSveEltType(const ASTContext &Ctx) const;
 
   /// Types are partitioned into 3 broad categories (C99 6.2.5p1):
   /// object types, function types, and incomplete types.
 

clang/lib/AST/TemplateBase.cpp

TemplateArgument::TemplateArgument(ASTContext &Ctx, const llvm::APSInt &Value,
   }
 
   Integer.Type = Type.getAsOpaquePtr();
 }
 
 TemplateArgument
 TemplateArgument::CreatePackCopy(ASTContext &Context,
                                  ArrayRef<TemplateArgument> Args) {
   if (Args.empty())
     return getEmptyPack();
 
   return TemplateArgument(Args.copy(Context));
 }
 
 TemplateArgumentDependence TemplateArgument::getDependence() const {
   auto Deps = TemplateArgumentDependence::None;
   switch (getKind()) {
case Declaration: {
     NamedDecl *ND = getAsDecl();
     if (getParamTypeForDecl()->isRecordType()) {
       if (auto *TPO = dyn_cast<TemplateParamObjectDecl>(ND)) {
         // FIXME: Include the type if it's not obvious from the context.
         TPO->printAsInit(Out);
         break;
       }
     }
-    if (!getParamTypeForDecl()->isReferenceType())
-      Out << '&';
+    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();
+        const clang::QualType InnerArgTy =
+            clang::QualType(ArgTy->getInnerMostPointerType(), 0);
+        const clang::QualType InnerParmTy =
+            clang::QualType(ParmTy->getInnerMostPointerType(), 0);
+        needsRef =
+            !(Ctx.hasSameType(decayedArgTy, ParmTy) ||
+              InnerArgTy.getAsString().compare(InnerParmTy.getAsString()) == 0);
+      }
+    }
+    if (!getParamTypeForDecl()->isReferenceType() && needsRef)
+      Out << "&";
     ND->printQualifiedName(Out);
     break;
   }
 
   case NullPtr:
     Out << "nullptr";
     break;
 

clang/lib/AST/Type.cpp

case BuiltinType::SveBool:
       return true;
     default:
       return false;
     }
   }
   return false;
 }
 
+const PointerType *Type::getInnerMostPointerType() const {
+  const PointerType *res = nullptr;
+  const Type *T = this;
+
+  while (true) {
+    const Type *pointeeOrElement = T->getPointeeOrArrayElementType();
+    if (pointeeOrElement == T)
+      // T is neither a pointer nor array type: we’re done.
+      break;
+
+    // T is a pointer or array type.
+    if (isa<PointerType>(T))
+      res = cast<PointerType>(T);
+
+    // iterate T, keep looking
+    T = pointeeOrElement;
+  }
+  return res;
+}
+
 QualType Type::getSveEltType(const ASTContext &Ctx) const {
   assert(isVLSTBuiltinType() && "unsupported type!");
 
   const BuiltinType *BTy = getAs<BuiltinType>();
   if (BTy->getKind() == BuiltinType::SveBool)
     // Represent predicates as i8 rather than i1 to avoid any layout issues.
     // The type is bitcasted to a scalable predicate type when casting between
     // scalable and fixed-length vectors.

clang/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>"
 
 template <typename A, typename B, typename C> struct ClassTemplate { A a; B b; C c; };
 ClassTemplate<char, short, ClassTemplate<int, int, int> > f;
 // This will only show up in normal debug builds.  The space in `> >` is
 // important for compatibility with Windows debuggers, so it should always be
 // there when generating CodeView.
 // UNQUAL-DAG: "ClassTemplate<char,short,ClassTemplate<int,int,int> >"

clang/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 = dso_local 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 = dso_local 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 = dso_local global %struct.TC zeroinitializer, align 1, !dbg [[TCN:![0-9]+]]
 // CHECK: @nn = dso_local 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:.*]], expr: !DIExpression())
 // 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,

clang/test/SemaTemplate/temp_arg_nontype_cxx11.cpp

struct StandardWidget {
     friend constexpr int WidgetCtor(X1*) {
       return 0;
     }
   };
   template struct StandardWidget<X1>;
 
   void use() { f<X1>(); }
 }
+
+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 {};
+
+constexpr const char **kZero[] = {};
+template <const char *const *const *, typename T> class Jolumn {};
+
+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,}}
+  Jolumn<kZero, double>().ls();   // expected-error {{<kZero,}}
+}