aaron.ballman marked 4 inline comments as done.
aaron.ballman added inline comments.
================
Comment at: clang/include/clang/AST/Type.h:1902
AutoTypeBitfields AutoTypeBits;
+ TypeOfBitfields TypeOfBits;
BuiltinTypeBitfields BuiltinTypeBits;
----------------
erichkeane wrote:
> So the downside to doing the bitfields is that EVERY 'Type' pays for them, or
> at least, pays for the largest one. As this is a rarely used and 'leaf' AST
> node, I would say using the bitfields for this is at minimum 'unnecessary'.
>
> That said, I doubt it is the 'biggest one', so I could go either way.
This is a union of objects, I'm certain all of them are at least one bit wide,
so we're not adding to any extra overhead, we're taking advantage of the
overhead we're already paying for.
================
Comment at: clang/lib/AST/ASTContext.cpp:12910-12916
+ // FIXME:: is this assumption correct or do we need to do work here to find
+ // the common type sugar regarding the stripped qualifiers if only one side
+ // is unqual?
+ assert(cast<TypeOfType>(X)->isUnqual() == cast<TypeOfType>(Y)->isUnqual()
&&
+ "typeof vs typeof_unqual mismatch?");
+ return Ctx.getTypeOfType(Ctx.getQualifiedType(Underlying),
+ cast<TypeOfType>(X)->isUnqual());
----------------
aaron.ballman wrote:
> mizvekov wrote:
> > mizvekov wrote:
> > > erichkeane wrote:
> > > > I'm unfamiliar with this function, but I would expect you MIGHT need
> > > > to? If only because they are the same AST node. Should 'unqual'
> > > > version be its own node? I'm on the fence, as it is a LOT of code to
> > > > do so, but also ends up being simpler in many places.
> > > A qualified and an unqualified typeof could have the same underlying
> > > type, so this assert can trip.
> > >
> > > I think what makes most sense is to unify them to a qualified typeof in
> > > case they differ, as that holds the underlying type unchanged:
> > > ```
> > > return Ctx.getTypeOfType(Ctx.getQualifiedType(Underlying),
> > > cast<TypeOfType>(X)->isUnqual() &&
> > > cast<TypeOfType>(Y)->isUnqual());
> > > ```
> > By the way, one thing to notice is the confusion regarding what is the
> > underlying type of this node, the result of `desugar()` or the result of
> > `getUnderlyingType()`?
> >
> > On my previous post, I meant the former.
> >
> > Maybe this is a good reason to rename `getUnderlyingType()` to
> > `getUnmodifiedType()` or something similar?
> FWIW: I still have to address these comments in the patch, but I'll do that
> tomorrow.
Thanks @mizvekov -- that makes sense to me. I've changed the implementation
here, and I've also renamed `getUnderlyingType()` to `getUnmodifiedType()`. I
was hesitant to do the rename before (but I was thinking about it!) because all
of the "this type represents another type" types use `getUnderlyingType()`
(such as `DecltypeType`, `UnaryTransformType`, etc). But in this case, the
"underlying type" is less clear because of the unqual variant, so I think
renaming is a good idea here.
================
Comment at: clang/lib/AST/Type.cpp:3474
+ : Type(TypeOfExpr,
+ Kind == TypeOfKind::Unqualified ? Can.getTypeofUnqualType() : Can,
toTypeDependence(E->getDependence()) |
----------------
erichkeane wrote:
> Does this have to be defensive about `Can` being the 'default' value of an
> empty qual-type?
`getTypeofUnqualType()` looks for a null type and handles it appropriately
already. If `Can` isn't valid, then we pass along the invalid type in either
branch.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134286/new/
https://reviews.llvm.org/D134286
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
