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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I'm on the fence about re-using the same nodes for `typeof` and `typeof_unqual`.  However, I have a preference to stop shuffling a bool around everywhere as parameters, and would like an enum (even if we store it as a bitfield : 1).  Something like:

  enum class TypeofKind {
         Typeof, TypeofUnqual };

I think improves readability in a bunch of places, AND gives us less work to do if we need to extend this node in the future.



================
Comment at: clang/include/clang/AST/ASTContext.h:1718
   /// GCC extension.
-  QualType getTypeOfExprType(Expr *e) const;
-  QualType getTypeOfType(QualType t) const;
+  QualType getTypeOfExprType(Expr *E, bool IsUnqual) const;
+  QualType getTypeOfType(QualType QT, bool IsUnqual) const;
----------------
Mild preference for an enum, instead of a bool here.

Alternatively, depending on your design below that I haven't seen yet, perhaps unqual versions of these functions?


================
Comment at: clang/include/clang/AST/Type.h:4542
 
-  TypeOfExprType(Expr *E, QualType can = QualType());
+  TypeOfExprType(Expr *E, bool IsUnqual, QualType Can = QualType());
 
----------------
This constructor is smelly.... Am I to guess this does something like, "if E, type is the type of this, else it is the type passed in Can?"


================
Comment at: clang/include/clang/AST/Type.h:4592
+          T->getDependence()), TOType(T), IsUnqual(IsUnqual) {
+    assert(!isa<TypedefType>(Can) && "Invalid canonical type");
   }
----------------
Are `TypedefType`'s EVER canonical?  Should you just be asserting 'Can' is canonical?


================
Comment at: clang/lib/AST/ASTContext.cpp:12910
   case Type::TypeOf:
-    return Ctx.getTypeOfType(Ctx.getQualifiedType(Underlying));
+    // 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
----------------
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.


================
Comment at: clang/lib/AST/Type.cpp:3502
+  // We strip all qualifier-like attributes as well.
+  if (const auto *AT =
+          dyn_cast_if_present<AttributedType>(Ret.getTypePtrOrNull());
----------------
In what situation is getTypePtrOrNull required here?  I would imagine that the 'isNull' check above should make sure we aren't in a situation where 'Ret' could be 'null' here?  Same with the 'if_present'.


================
Comment at: clang/lib/Lex/Preprocessor.cpp:61
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSwitch.h"
 #include "llvm/Support/Capacity.h"
----------------
Oh?


================
Comment at: clang/lib/Parse/ParseDecl.cpp:7463
   SourceLocation StartLoc = ConsumeToken();
-
-  const bool hasParens = Tok.is(tok::l_paren);
+  bool HasParens = Tok.is(tok::l_paren);
 
----------------
Should it be an error to `!HasParens` if `IsUnqual`.


================
Comment at: clang/lib/Sema/SemaType.cpp:9137
+    Diag(E->getExprLoc(), diag::err_sizeof_alignof_typeof_bitfield)
+        << (IsUnqual ? 3 : 2);
 
----------------
`(IsUnqual + 2)` maybe?  Or perhaps too cute.


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

https://reviews.llvm.org/D134286