[PATCH] D40508: Replace long type names in IR with hashes

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

It's not clear to me that this abbreviation functionality should live in Support. Clang probably wants enough control over this (assuming we're doing it) that the logic should live in clang.

I also think we might want to try solving this a completely different way: just don't bother emitting more than two template arguments for IR type names. We don't really need to worry about type name uniqueness or matching them across TUs.



================
Comment at: lib/AST/TypePrinter.cpp:1532-1534
+namespace {
+template<typename TA>
+void printTo(raw_ostream &OS, ArrayRef<TA> Args, const PrintingPolicy &Policy,
----------------
`static` is nicer than a short anonymous namespace.


================
Comment at: lib/AST/TypePrinter.cpp:1543
+  for (const auto &Arg : Args) {
+    std::unique_ptr<SmallString<128>> Buffer;
+    std::unique_ptr<llvm::raw_svector_ostream> BufOS;
----------------
Just use `SmallString<0>` for Buffer. No wasted stack space, no extra unique_ptr. It will allocate memory if it needs it.


================
Comment at: lib/AST/TypePrinter.cpp:1588-1589
 
-// Sadly, repeat all that with TemplateArgLoc.
-void TemplateSpecializationType::
-PrintTemplateArgumentList(raw_ostream &OS,
-                          ArrayRef<TemplateArgumentLoc> Args,
-                          const PrintingPolicy &Policy) {
-  OS << '<';
-  const char *Comma = Policy.MSVCFormatting ? "," : ", ";
-
-  bool needSpace = false;
-  bool FirstArg = true;
-  for (const TemplateArgumentLoc &Arg : Args) {
-    if (!FirstArg)
-      OS << Comma;
-
-    // Print the argument into a string.
-    SmallString<128> Buf;
-    llvm::raw_svector_ostream ArgOS(Buf);
-    if (Arg.getArgument().getKind() == TemplateArgument::Pack) {
-      PrintTemplateArgumentList(ArgOS,
-                                Arg.getArgument().getPackAsArray(),
-                                Policy, true);
-    } else {
-      Arg.getArgument().print(Policy, ArgOS);
-    }
-    StringRef ArgString = ArgOS.str();
-
-    // If this is the first argument and its string representation
-    // begins with the global scope specifier ('::foo'), add a space
-    // to avoid printing the diagraph '<:'.
-    if (FirstArg && !ArgString.empty() && ArgString[0] == ':')
-      OS << ' ';
-
-    OS << ArgString;
-
-    needSpace = (!ArgString.empty() && ArgString.back() == '>');
-    FirstArg = false;
-  }
+namespace clang {
+void printTemplateArgumentList(raw_ostream &OS,
+                               const TemplateArgumentListInfo &Args,
----------------
It's usually nicer to define free functions in namespaces as `void clang::printTemplateArgumentList(...`. This ensures that nobody can mess up the namespace scope or forget to include the header that declares the function. It also sometimes turns link errors into compile errors.


https://reviews.llvm.org/D40508