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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
