rsmith added inline comments.
================
Comment at: clang/lib/AST/ItaniumMangle.cpp:3957
switch (T->getUTTKind()) {
case UnaryTransformType::EnumUnderlyingType:
----------------
I'd prefer that you restructure this to first compute a `StringRef`
corresponding to the name of the trait and then use `Out << 'u' << Name.size()
<< Name`. (That'd make the miscounting error that a previous version of this
patch had with `__remove_pointer` impossible.)
================
Comment at: clang/lib/AST/JSONNodeDumper.cpp:670
+ case UnaryTransformType::AddLvalueReference:
+ JOS.attribute("transformedKind", "add_lvalue_reference");
+ break;
----------------
`__underlying_type` has a property called `transformKind`; here you're using
the name `transformedKind` instead. I think the non-`ed` name makes more sense.
================
Comment at: clang/lib/AST/TextNodeDumper.cpp:1553
+ case UnaryTransformType::AddLvalueReference:
+ OS << " add_lvalue_reference";
+ break;
----------------
This is the third time we've hardcoded a mapping between enumerators and names
(and it looks like there are more to follow). That seems enough to be worth
adding a .def file to avoid repeating this mapping.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:4149-4168
case tok::kw___underlying_type:
ParseUnderlyingTypeSpecifier(DS);
continue;
+ case tok::kw___add_lvalue_reference:
+ case tok::kw___add_pointer:
+ case tok::kw___add_rvalue_reference:
----------------
Do we need different handling for `__underlying_type` versus the other unary
type transforms here?
================
Comment at: clang/lib/Parse/ParseDecl.cpp:4287
+ default:
+ llvm_unreachable(__FILE__
+ "passed in an unhandled type transformation built-in");
----------------
If we want a file name in `llvm_unreachable` and it's not providing one, I
think that's something we should add centrally rather than in this one usage --
I'd prefer that you not pass `__FILE__` here and instead produce a separate
patch to extend `llvm_unreachable` if needed. (Also you seem to be missing any
whitespace in the resulting string between the filename and the start of the
message.)
================
Comment at: clang/lib/Parse/ParseDecl.cpp:4297
+ if (T.expectAndConsume(diag::err_expected_lparen_after,
+ "type transformation builtin", tok::r_paren))
+ return;
----------------
Do not pass English-language text fragments to diagnostics -- we aim for our
diagnostics to (at least in principle) be translatable. Instead, add a new
diagnostic for this situation (or perhaps just use `diag::err_expected,
tok::l_paren`).
================
Comment at: clang/lib/Parse/ParseDecl.cpp:4317
+ }
+ DS.setTypeofParensRange(T.getRange());
+}
----------------
Can we give this a better name, given that it's not used for only `typeof`?
Maybe something like `setTypeArgumentRange`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116203/new/
https://reviews.llvm.org/D116203
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
