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

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added a comment.

Thanks for patiently reviewing! I'll do the libc++ stuff and a pilot run in some code. Hopefully can get it merged tomorrow :-)



================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:1149-1150
+  default:
+    assert(false && "passed in an unhandled type transformation built-in");
+    llvm_unreachable("passed in an unhandled type transformation built-in");
+  }
----------------
rsmith wrote:
> rsmith wrote:
> > We don't need both of these. Just the `llvm_unreachable` would suffice.
> This comment still needs to be addressed.
Could've sworn I did this one, but it might've gotten mixed up in the D116280 switcheroo.


================
Comment at: clang/lib/Parse/ParseExpr.cpp:1066-1067
 
+          REVERTIBLE_TYPE_TRAIT(__add_lvalue_reference);
+          REVERTIBLE_TYPE_TRAIT(__add_rvalue_reference);
           REVERTIBLE_TYPE_TRAIT(__is_abstract);
----------------
rsmith wrote:
> Just curious: why do we only handle six of the traits here, rather than all of them?
Why indeed. Perhaps these were the original six I worked on and didn't come back to update this.


================
Comment at: clang/lib/Parse/ParseExpr.cpp:1750
+#undef TRANSFORM_TYPE_TRAIT_DEF
+    if (NextToken().is(tok::less)) {
+      Tok.setKind(tok::identifier);
----------------
cjdb wrote:
> rsmith wrote:
> > cjdb wrote:
> > > rsmith wrote:
> > > > Here you're checking for `trait<` and treating it as an identifier; elsewhere you're checking for `trait(` and treating it as a builtin. Is there a reason to treat `trait` followed by a token other than `(` or `<` inconsistently?
> > > The parser needs to treat `trait<` as an identifier to accommodate libstdc++'s usage of a few of these as alias templates. I've added a comment to explain why in the code.
> > I agree we need to treat `trait<` as an identifier and `trait(` as the builtin. My question is, why do we have inconsistent treatment of the remaining cases (`trait` followed by any other token)? For example, `MaybeParseTypeTransformTypeSpecifier` treats `trait + 1` as an identifier. But this code treats it as the builtin name.
> > 
> > I think there are two choices that make the most sense, if they work:
> > 
> > 1) `trait(` is the builtin and any other utterance is an identifier, or
> > 2) `trait<` is an identifier, `using trait =` is an identifier, and any other utterance is the builtin.
> > 
> > I think I prefer option (2), given that it's defining the narrower special case. But all I'm really looking for here is a consistent story one way or the other, if it's feasible to have one.
> I'd like for it to be (2) as well, but based on how we do alias-declarations, I'm concerned that will have performance implications for a rare occurrence.
You haven't countered this concern, so I'm going to close for now and will happily revisit if this one just slipped by unnoticed.


================
Comment at: clang/lib/Sema/SemaType.cpp:9375
+      });
+  assert(Result != Consider->end());
+
----------------
rsmith wrote:
> Can this fail for an enum whose underlying type is `_BitInt(N)` for some unusual `N`?
Nice catch. This wasn't supposed to happen, but I ended up changing the logic so that `_BitInt` is considered here instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203