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