[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

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

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