[libcxx-commits] [PATCH] D67052: Add reference type transformation builtins

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 6 inline comments as done.
zoecarver added a comment.

@EricWF and @miscco thanks for the review comments. Sorry for the delay, I forgot about this patch. All your comments have been addressed/fixed.



================
Comment at: clang/lib/AST/TypePrinter.cpp:1020
 
   switch (T->getUTTKind()) {
     case UnaryTransformType::EnumUnderlyingType:
----------------
miscco wrote:
> Couldn't we use `TextNodeDumper::VisitUnaryTransformType` to dump the string and then simplify it to a single case.
> 
> Given the possibility that maybe some day one might add the `add_foo` builtins
> Couldn't we use TextNodeDumper::VisitUnaryTransformType to dump the string and then simplify it to a single case.

Good call. Will do.

> Given the possibility that maybe some day one might add the add_foo builtins

I had a patch that added those builtins but, we decided that there was no real reason for them to exist.



================
Comment at: clang/lib/Sema/SemaType.cpp:1624
   }
+  case DeclSpec::TST_removeReferenceType:
+  case DeclSpec::TST_removeCV:
----------------
EricWF wrote:
> Why can't we share the implementation with `TST_underlyingType`?
Yep, fixed. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67052