[PATCH] D77598: Integral template argument suffix and cast printing

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Thanks, I'm broadly very happy with this. My remaining comments are all very minor.



================
Comment at: clang/include/clang/AST/Expr.h:1619
 
+  static void streamChar(unsigned val, CharacterKind Kind, raw_ostream &OS) {
+    switch (Kind) {
----------------
This is sufficiently large that it should be in the `.cpp` file not in the header. I think calling this `print` would be more in line with our normal conventions. Please capitalize `val` as `Val`.


================
Comment at: clang/lib/AST/DeclTemplate.cpp:174
+    return true;
+  else {
+    const NamedDecl *TemplParam = TPL->getParam(Idx);
----------------
Style nit: no need for `else` when the `if` side `return`s.


================
Comment at: clang/lib/AST/TemplateBase.cpp:90-92
     Out << ((Ch == '\'') ? "'\\" : "'");
     Out.write_escaped(StringRef(&Ch, 1), /*UseHexEscapes=*/ true);
     Out << "'";
----------------
Can we use `CharacterLiteral::streamChar` here too?


================
Comment at: clang/lib/AST/TypePrinter.cpp:1996-1997
                     const PrintingPolicy &Policy, bool SkipBrackets,
-                    const TemplateParameterList *TPL) {
+                    const TemplateParameterList *TPL, bool isPack,
+                    unsigned parmIndex) {
   // Drop trailing template arguments that match default arguments.
----------------
Please capitalize `isPack` and `parmIndex`.


================
Comment at: clang/lib/AST/TypePrinter.cpp:2001
       !Policy.PrintCanonicalTypes && !Args.empty() &&
       Args.size() <= TPL->size()) {
     ASTContext &Ctx = TPL->getParam(0)->getASTContext();
----------------
This should have a `!isPack` condition -- we don't need this because there can't be default arguments for a parameter pack anyway, and we don't want it because it's looking at the wrong parameters in `TPL`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77598/new/

https://reviews.llvm.org/D77598