[CIR][NFC] Simplify struct type name creation

[andykaylor]

During review of a patch for upstreaming the cir.struct type support, Erich Keane observed that the code we use for creating our type name for structures with templates was likely to be error prone. He recommended using getNameForDiagnostic instead.

After some experimentation, I found that with a couple of changes to the printing policy, I could get the same strings we currently produce by calling getNameForDiagnostic. This change does that.

Erich also pointed out that RecordDecls always have a DeclContext so a few other lines could be eliminated where that was checked.

[erichkeane]

PROBABLY don't actually want to use getNameForDiagnostic, as that isn't a particularly stable output. But it effectively uses the TypePrinter, which you can get to by QualType::print method (or QualType::getAsString).

We probably ALSO instead of using the recordDecl, we ACTUALLY want to get its type, and canonicalize it. So you probably instead want to be doing:

ASTContext.getRecordType(recordDecl)->print(....) https://clang.llvm.org/doxygen/classclang_1_1QualType.html#a84a489ae72024250d02b58a0cf7e624c

[andykaylor]

Hmmm, not stable you say? I didn't expect that. As I was trying to figure out how to get this to do what you suggested, I looked at the ClassTemplateSpecializationDecl::getNameForDiagnostic() implementation,

  NamedDecl::getNameForDiagnostic(OS, Policy, Qualified);
 
  const auto *PS = dyn_cast<ClassTemplatePartialSpecializationDecl>(this);
  if (const ASTTemplateArgumentListInfo *ArgsAsWritten =
          PS ? PS->getTemplateArgsAsWritten() : nullptr) {
    printTemplateArgumentList(
        OS, ArgsAsWritten->arguments(), Policy,
        getSpecializedTemplate()->getTemplateParameters());
  } else {
    const TemplateArgumentList &TemplateArgs = getTemplateArgs();
    printTemplateArgumentList(
        OS, TemplateArgs.asArray(), Policy,
        getSpecializedTemplate()->getTemplateParameters());
  }

I had everything after the call to NamedDecl::getNameForDiagnostic in place after the dyn_cast to ClassTemplateSpecializationDecl and that works, but I thought if I were doing that I may as well just call getNameForDiagnostic because the rest of it doesn't look obviously better than what is already there. I'm pretty sure the call to arg.print() will end up going through the TypePrinter too, though I'm not clear why the code above needs me to set policy.PrintCanonicalTypes but the original code doesn't. Without that, the code above (and the code in this PR) prints coroutine__handle<> instead of coroutine__handle<void> in one of the tests and, for reasons I don't understand, that causes us to drop an alloca.

[erichkeane]

Hmmm, not stable you say?
Yeah, our diagnostics printing is often altered.

I'm not clear why the code above needs me to set policy.PrintCanonicalTypes
Because else you can get typedefs/etc in the print, which means you could get 2 identical types with different string representations otherwise.

prints coroutine__handle<> instead of coroutine__handle
coroutine__handle<> is incorrect, coroutine__handle<void> is correct I think? Since there IS a type there. There might be a policy choice to switch.

My biggest fear of trying to 'roll our own' is that every time we end up adding an ast node that changes how template args, or how the language works (like with concepts, etc), that the string format is going to need to be updated, and it won't be clear how. So this will necessarily get out of sync.

[andykaylor]

prints coroutine__handle<> instead of coroutine__handle
coroutine__handle<> is incorrect, coroutine__handle<void> is correct I think? Since there IS a type there. There might be a policy choice to switch.

Right. There is a policy setting to get it to show up. I just don't understand why the original code here didn't need it.

My biggest fear of trying to 'roll our own' is that every time we end up adding an ast node that changes how template args, or how the language works (like with concepts, etc), that the string format is going to need to be updated, and it won't be clear how. So this will necessarily get out of sync.

That makes sense, but it's starting to seem like we could run into problems like that whenever the TypePrinter gets updated, but maybe that's just unavoidable. I'll see what I can do with your suggestion to use QualType::print.

[andykaylor]

This also works:

    policy.SuppressTagKeyword = true;
    astContext.getRecordType(recordDecl).print(outStream, policy);

That is in addition to the other policy adjustments in this PR. Would that be more stable long term?

[erichkeane]

Its definitely more stable than the rest of the ways, and has some level of guarantees of stability.

[bcardosolopes]

I'm in favor of this change. Some extra context here: in the past we wanted to incrementally check before printing because the printing function (as you observed) has its inner logic depending on the AST node, and wanted to NYI before that - that clearly doesn't make sense anymore. Thanks for improving this!