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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 13 inline comments as done.
aaron.ballman added a comment.

In D134286#3803077 <https://reviews.llvm.org/D134286#3803077>, @erichkeane wrote:

> 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.

I'm happy to switch to an enum for this. I don't think it makes a whole lot of sense to add additional types for `typeof_unqual` support though. Using inheritance would be a bit risky (someone may accidentally use a `typeof_unqual` type as a `typeof`), and there's only one bit of difference between the two types.



================
Comment at: clang/include/clang/AST/ASTContext.h:1717
 
   /// GCC extension.
+  QualType getTypeOfExprType(Expr *E, bool IsUnqual) const;
----------------
shafik wrote:
> I guess these are not purely extensions anymore?
Good catch!


================
Comment at: clang/include/clang/AST/Type.h:4537
   Expr *TOExpr;
+  bool IsUnqual;
 
----------------
mizvekov wrote:
> You can use the Type Bitfields in order to avoid bumping the size of the node.
> 
Good call!


================
Comment at: clang/include/clang/AST/Type.h:4542
 
-  TypeOfExprType(Expr *E, QualType can = QualType());
+  TypeOfExprType(Expr *E, bool IsUnqual, QualType Can = QualType());
 
----------------
erichkeane wrote:
> 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?"
No, it's more that `E` holds the type and `Can` holds the canonical type, which can sometimes be different than the type of `E` in the case of dependent typeof expressions. See `ASTContext::getTypeOfExprType()` for the interesting use.


================
Comment at: clang/include/clang/AST/Type.h:4601
+    QualType QT = getUnderlyingType();
+    return IsUnqual ? QT.getTypeofUnqualType() : QT;
+  }
----------------
mizvekov wrote:
> shafik wrote:
> > It is interesting that you do `getTypeofUnqualType` in the constructor and then you have to do it again when desuguring. Is this tested in the tests?
> The constructor takes the canonical type, while this preserves the unmodified type (which can be accessed through getUnderlyingType).
Yup, this is covered in the tests. `desugar()` gets called as part of type merging, and the canonical type is used when printing `aka` information.


================
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());
----------------
erichkeane wrote:
> 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'.
This was a hold over from before I had the `isNull()` check above, so I've fixed this up.


================
Comment at: clang/lib/Lex/Preprocessor.cpp:61
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSwitch.h"
 #include "llvm/Support/Capacity.h"
----------------
erichkeane wrote:
> Oh?
Wth? No idea how that got here, but it's gone now.


================
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);
 
----------------
erichkeane wrote:
> Should it be an error to `!HasParens` if `IsUnqual`.
I don't think we can get here in C2x mode, and `typeof_unqual` is not exposed outside of that mode. We've got test coverage for this (in C2x):
```
typeof 0 int i = 12;         // expected-error {{expected '(' after 'typeof'}} expected-error {{expected identifier or '('}}
typeof 0 j = 12;             // expected-error {{expected '(' after 'typeof'}} expected-error {{expected identifier or '('}}
typeof_unqual 0 k = 12;      // expected-error {{expected '(' after 'typeof_unqual'}} expected-error {{expected identifier or '('}}
typeof_unqual 0 int l = 12;  // expected-error {{expected '(' after 'typeof_unqual'}} expected-error {{expected identifier or '('}}
```



================
Comment at: clang/lib/Sema/SemaType.cpp:9137
+    Diag(E->getExprLoc(), diag::err_sizeof_alignof_typeof_bitfield)
+        << (IsUnqual ? 3 : 2);
 
----------------
erichkeane wrote:
> `(IsUnqual + 2)` maybe?  Or perhaps too cute.
LoL far too cute, but this will change now that I'm using an enum more.


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

https://reviews.llvm.org/D134286