[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Apr 10 13:34:31 PDT 2018
aaron.ballman added a comment.
Thank you for working on this!
================
Comment at: include/clang/AST/ComparisonCategories.h:78
+ const DeclRefExpr *getResultValue(ComparisonCategoryResult ValueKind) const {
+ const auto *DR = getResultValueUnsafe(ValueKind);
+ assert(DR &&
----------------
Avoid `auto` when the type is not explicitly spelled out in the initialization.
================
Comment at: include/clang/AST/ComparisonCategories.h:155-158
+ const ComparisonCategoryInfo &getInfo(ComparisonCategoryKind Kind) const {
+ assert(HasData && "comparison category information not yet built");
+ return Data[static_cast<unsigned>(Kind)];
+ }
----------------
It seems like this method can be private rather than public.
================
Comment at: include/clang/AST/ComparisonCategories.h:170
+
+ /// \brief Return the comparison category kind coorisponding to the specified
+ /// type. 'Ty' is expected to refer to the type of one of the comparison
----------------
s/coorisponding/corresponding
================
Comment at: include/clang/AST/ComparisonCategories.h:176-177
+ /// \brief returns true if the comparison category data has already been
+ /// built,
+ /// and false otherwise.
+ bool hasData() const { return HasData; }
----------------
Did this get re-flowed in a strange manner?
================
Comment at: include/clang/AST/ComparisonCategories.h:180
+
+public: // private
+ using InfoList =
----------------
The comment here seems incorrect.
================
Comment at: include/clang/AST/Expr.h:3020
+ // ordered comparison. Otherwise the comparison is an equality comparison.
+ unsigned IsCmpOrdered : 1;
+
----------------
It's unfortunate that this means we're using 9 bits rather than the 8 we were previously using, but I don't see a particularly good way to solve that.
================
Comment at: include/clang/AST/Expr.h:3028
BinaryOperator(Expr *lhs, Expr *rhs, Opcode opc, QualType ResTy,
- ExprValueKind VK, ExprObjectKind OK,
- SourceLocation opLoc, FPOptions FPFeatures)
+ ExprValueKind VK, ExprObjectKind OK, SourceLocation opLoc,
+ FPOptions FPFeatures)
----------------
s/opLoc/OpLoc
================
Comment at: include/clang/AST/Expr.h:3167
+ void setIsCmpOrdered(bool Val = true) { IsCmpOrdered = Val; }
+ bool getIsCmpOrdered() const { return IsCmpOrdered; }
----------------
Both calls to this function are explicit with the argument, so I think you can drop the default argument value.
================
Comment at: include/clang/Sema/Sema.h:4547
+ /// results are cached in ASTContext so they are accessible outside of Sema.
+ /// An error is emitted if the types are not found or another error occurs.
+ ///
----------------
Extra whitespace between emitted and if.
================
Comment at: lib/AST/ComparisonCategories.cpp:1
+//===- ComparisonCategories.h - Three Way Comparison Data -------*- C++ -*-===//
+//
----------------
.cpp instead of .h
================
Comment at: lib/AST/ComparisonCategories.cpp:38
+ComparisonCategories::getInfoForType(QualType Ty) const {
+ auto OptKind = getCategoryForType(Ty);
+ assert(OptKind &&
----------------
Don't use `auto` here.
================
Comment at: lib/AST/ExprConstant.cpp:5084
+
+ BinOpEvaluatorBase(EvalInfo &info, ComparisonCategoryResult *CmpRes = nullptr)
+ : ExprEvaluatorBaseTy(info), CmpResult(CmpRes) {
----------------
s/info/Info
================
Comment at: lib/AST/ExprConstant.cpp:6301
+ using CCR = ComparisonCategoryResult;
+ auto &CmpInfo = Info.Ctx.CompCategories.getInfoForType(E->getType());
+
----------------
Don't use `auto` here.
================
Comment at: lib/AST/ExprConstant.cpp:8589
+ bool IsSpaceship = E->getOpcode() == BO_Cmp;
+ auto &Info = this->Info;
// We don't call noteFailure immediately because the assignment happens after
----------------
Don't use `auto` here. Also, perhaps the two variables can be sunk closer to where they're used?
================
Comment at: lib/AST/ExprConstant.cpp:8930
if (LHSTy->isMemberPointerType()) {
- assert(E->isEqualityOp() && "unexpected member pointer operation");
+ assert((E->isEqualityOp()) && "unexpected member pointer operation");
assert(RHSTy->isMemberPointerType() && "invalid comparison");
----------------
Spurious parens?
================
Comment at: lib/AST/ExprConstant.cpp:10814
+ // Check that all of the references to the result objects are ICE.
+ auto &CmpInfo = Ctx.CompCategories.getInfoForType(Exp->getType());
+ ICEDiag RetDiag(IK_ICE, E->getLocStart());
----------------
Don't use `auto` here.
================
Comment at: lib/AST/ExprConstant.cpp:10816
+ ICEDiag RetDiag(IK_ICE, E->getLocStart());
+ for (auto &KV : CmpInfo.Objects)
+ RetDiag = Worst(RetDiag, CheckICE(KV.second, Ctx));
----------------
`const auto &`?
================
Comment at: lib/CodeGen/CGExprAgg.cpp:892
+ QualType ArgTy = E->getLHS()->getType();
+ if (auto *MPT = ArgTy->getAs<MemberPointerType>()) {
+ assert(Kind == CK_NonEqual &&
----------------
`const auto *`?
================
Comment at: lib/CodeGen/CGExprAgg.cpp:925-929
+ } else if (ArgTy->isIntegralOrEnumerationType() || ArgTy->isPointerType()) {
+ auto Inst =
+ ArgTy->hasSignedIntegerRepresentation() ? InstInfo.SCmp : InstInfo.UCmp;
+ return Builder.CreateICmp(Inst, LHS, RHS, "cmp");
+ } else if (ArgTy->isAnyComplexType()) {
----------------
You can remove the `else` after a return and just turn these into `if` statements.
================
Comment at: lib/CodeGen/CGExprAgg.cpp:950
+
+ auto &CmpInfo =
+ CGF.getContext().CompCategories.getInfoForType(E->getType());
----------------
Don't use `auto` here. I'll stop mentioning these for now -- basically, the general rule we follow is to use auto only when the type is explicitly spelled out in the initialization or if the type is horribly obvious by context and annoying to spell out (like iterators).
================
Comment at: lib/Sema/SemaDeclCXX.cpp:8965-8967
+ std::pair<char, DeclRefExpr *> KV{(char)CCV,
+ cast<DeclRefExpr>(Res.get())};
+ Info.Objects.insert(KV);
----------------
Use `make_pair()` instead of a local variable? Or would it be better to just call `try_emplace()`?
================
Comment at: lib/Sema/SemaExpr.cpp:9733
+ SCS.setToType(1, ToType);
+ if (ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(E)) {
+ auto CastK = ICE->getCastKind();
----------------
`const auto *`
================
Comment at: lib/Sema/SemaExpr.cpp:9761-9763
+ if (E->isValueDependent()) {
+ return false;
+ }
----------------
Can elide braces.
================
Comment at: lib/Sema/SemaExpr.cpp:9770
+
+ if ((ToType->isReferenceType()
+ ? !E->EvaluateAsLValue(Eval, S.Context)
----------------
Spurious parens.
================
Comment at: lib/Sema/SemaExpr.cpp:9776-9781
+ } else {
+ if (Notes.empty()) {
+ // It's a constant expression.
+ return false;
+ }
+ }
----------------
Can be combined into an `else if`.
================
Comment at: lib/Sema/SemaExpr.cpp:11968-11971
+ if (!ResultTy.isNull()) {
+ IsCmpOrdered =
+ Context.CompCategories.getInfoForType(ResultTy).isOrdered();
+ }
----------------
Can elide braces.
https://reviews.llvm.org/D45476
More information about the cfe-commits
mailing list