This is an archive of the discontinued LLVM Phabricator instance.

Fix type printing of array template args
ClosedPublic

Authored by reikdas on Aug 5 2017, 5:58 PM.

Details

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?

Diff Detail

Event Timeline

karies created this revision.Aug 5 2017, 5:58 PM
rjmccall edited edge metadata.Aug 6 2017, 11:06 PM

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 updated this revision to Diff 109963.Aug 7 2017, 2:57 AM

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 updated this revision to Diff 109965.Aug 7 2017, 3:00 AM

...and remove FIXME.

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.

Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2019, 10:05 PM
reikdas commandeered this revision.Nov 2 2020, 3:00 AM
reikdas added a reviewer: karies.
reikdas updated this revision to Diff 302234.Nov 2 2020, 3:02 AM

Addresses @rjmccall 's comment

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 updated this revision to Diff 340736.Apr 27 2021, 12:11 AM

Address comments.

reikdas marked 2 inline comments as done.Apr 27 2021, 12:12 AM
rjmccall added inline comments.Apr 27 2021, 10:04 AM
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 updated this revision to Diff 341424.Apr 29 2021, 12:40 AM

Address @rjmccall inline comment.

reikdas marked an inline comment as done.Apr 29 2021, 12:40 AM
rjmccall added inline comments.Apr 29 2021, 1:17 AM
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 added inline comments.Apr 29 2021, 3:30 AM
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 added inline comments.Apr 29 2021, 11:42 AM
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 updated this revision to Diff 341775.Apr 29 2021, 11:13 PM

Address @rjmccall inline comment.

reikdas marked 2 inline comments as done.Apr 29 2021, 11:13 PM
rjmccall added inline comments.Apr 29 2021, 11:53 PM
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 updated this revision to Diff 341841.Apr 30 2021, 4:22 AM

Address @rjmccall 's inline comment.

rjmccall accepted this revision.Apr 30 2021, 11:55 AM

Thanks, LGTM

This revision is now accepted and ready to land.Apr 30 2021, 11:55 AM
This revision was automatically updated to reflect the committed changes.