[PATCH] D134286: [C2x] implement typeof and typeof_unqual

[Aaron Ballman via Phabricator via cfe-commits]

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