[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Apr 11 13:07:33 PDT 2022
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
More information about the cfe-commits
mailing list