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