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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
