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

404

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

87

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 ↗

(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

clang/lib/AST/TemplateBase.cpp

if (T->isBooleanType() && !Policy.MSVCFormatting) {
     Out << ((Ch == '\'') ? "'\\" : "'");
     Out.write_escaped(StringRef(&Ch, 1), /*UseHexEscapes=*/ true);
     Out << "'";
   } else {
     Out << Val;
   }
 }
 
+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) {
+  // 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 paramType->isMemberPointerType();
+  if (argType->isArrayType())
+    return getArrayDepth(argType) == getArrayDepth(paramType->getPointeeType());
+  return true;
+}
+
 //===----------------------------------------------------------------------===//
 // TemplateArgument Implementation
 //===----------------------------------------------------------------------===//
 
 TemplateArgument::TemplateArgument(ASTContext &Ctx, const llvm::APSInt &Value,
                                    QualType Type) {
   Integer.Kind = Integral;
   // Copy the APSInt value into our decomposed form.
   Integer.BitWidth = Value.getBitWidth();
   Integer.IsUnsigned = Value.isUnsigned();
   // If the value is large, we have to get additional memory from the ASTContext
   unsigned NumWords = Value.getNumWords();
   if (NumWords > 1) {
     void *Mem = Ctx.Allocate(NumWords * sizeof(uint64_t));
     std::memcpy(Mem, Value.getRawData(), NumWords * sizeof(uint64_t));
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 << '&';
+    if (auto *VD = dyn_cast<ValueDecl>(ND)) {
+      if (needsAmpersandOnTemplateArg(getParamTypeForDecl(), VD->getType()))
+        Out << "&";
+    }
     ND->printQualifiedName(Out);
     break;
   }
 
   case NullPtr:
     Out << "nullptr";
     break;
 
   case Template:
     getAsTemplate().print(Out, Policy);
     break;
 
   case TemplateExpansion:
     getAsTemplateOrTemplatePattern().print(Out, Policy);
     Out << "...";
     break;
 
   case Integral:
     printIntegral(*this, Out, Policy);
     break;
 
   case Expression:
     getAsExpr()->printPretty(Out, nullptr, Policy);

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

 // CHECK-DAG: "operator delete"
 // CHECK-DAG: "operator delete[]"
 // CHECK-DAG: "operator+"
 
 template <typename T, void (*)(void)>
 void fn_tmpl() {}
 
 template void fn_tmpl<int, freefunc>();
 // CHECK-DAG: "fn_tmpl<int,&freefunc>"
 
+template <typename T, void (*)(void)>
+void fn_tmpl_typecheck() {}
+
+template void fn_tmpl_typecheck<int, &freefunc>();
+// CHECK-DAG: "fn_tmpl_typecheck<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/SemaTemplate/temp_arg_nontype_cxx11.cpp

namespace ReferenceToConstexpr {
   template<const A &r, typename T> struct B {
     static_assert(__builtin_strcmp(r.str, "hello") == 0, "");
   };
   template<const A &r> struct C {
     template<typename T> void f(B<r, T>, T) {}
   };
   void f(C<a> ca) { ca.f({}, 0); }
 }
+
+using FourChars = const char[4];
+constexpr FourChars kEta = "Eta";
+constexpr const char kDes[4] = "Des";
+constexpr const char *kNull = "Phi";
+constexpr const char **kZero[] = {};
+
+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 {};
+template <const char *const *const *, typename T> class Jolumn {};
+template <const char **const (*)[0], typename T> class Iolumn {};
+
+class container {
+public:
+  int a;
+};
+template <int container::*> class Kolumn {};
+
+void lookup() {
+  Column<kEta, double>().ls();    // expected-error {{<kEta,}}
+  Column<kDes, double>().ls();    // expected-error {{<kDes,}}
+  Column<nullptr, double>().ls(); // expected-error {{<nullptr,}}
+  Dolumn<kEta, double>().ls();    // expected-error {{<kEta,}}
+  Dolumn<kDes, double>().ls();    // expected-error {{<kDes,}}
+  Folumn<&kEta, double>().ls();   // expected-error {{<&kEta,}}
+  Folumn<&kDes, double>().ls();   // expected-error {{<&kDes,}}
+  Golumn<&kEta, double>().ls();   // expected-error {{<&kEta,}}
+  Golumn<&kDes, double>().ls();   // expected-error {{<&kDes,}}
+  Holumn<&kNull, double>().ls();  // expected-error {{<&kNull,}}
+  Jolumn<kZero, double>().ls();   // expected-error {{<kZero,}}
+  Iolumn<&kZero, double>().ls();  // expected-error {{<&kZero,}}
+  Kolumn<&container::a>().ls();   // expected-error {{<&container::a}}
+  Kolumn<nullptr>().ls();         // expected-error {{<nullptr}}
+}