[PATCH] D131416: [Clang][BinaryOperator] cache ICEKind

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.


================
Comment at: clang/include/clang/AST/Expr.h:30
 #include "clang/Basic/TypeTraits.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/APFloat.h"
----------------
Please keep the list of `#include`s alphabetically sorted.


================
Comment at: clang/include/clang/AST/Expr.h:3808
 /// be used to express the computation.
+namespace {
+
----------------
Let's keep additions before the comment block specific to `BinaryOperator`'s definition.


================
Comment at: clang/include/clang/AST/Expr.h:3821-3826
+struct ICEDiag {
+  ICEKind Kind;
+  SourceLocation Loc;
+
+  ICEDiag(ICEKind IK, SourceLocation l) : Kind(IK), Loc(l) {}
+};
----------------
Move back.


================
Comment at: clang/include/clang/AST/Expr.h:3837
+  /* ICEDiag Diag{IK_NotYetEvaluated, SourceLocation()}; */
+  llvm::Optional<enum ICEKind> IK;
+  /* ICEKind IK = IK_NotYetEvaluated; */
----------------
Let's make this private, initialize it to `llvm::None` (see llvm/include/llvm/ADT/None.h), then add getters and setters.  They should accept/return `ICEKind`; basically, I don't think we should support un-setting the ICEKind once we know what it is.

Also, prefer `ICEKind` in C++; `enum ICEKind` is required only in C.


================
Comment at: clang/lib/AST/ExprConstant.cpp:15516
+    BinaryOperator *Exp = cast<BinaryOperator>(E);
+    if (Exp->IK.has_value()) return ICEDiag(Exp->IK.value(), E->getBeginLoc());
+    /* if (Exp->IK != IK_NotYetEvaluated) return ICEDiag(Exp->IK, E->getBeginLoc()); */
----------------
make this two lines?

`$ git-clang-format HEAD~` should reformat this for you.


================
Comment at: clang/lib/AST/ExprConstant.cpp:15517
+    if (Exp->IK.has_value()) return ICEDiag(Exp->IK.value(), E->getBeginLoc());
+    /* if (Exp->IK != IK_NotYetEvaluated) return ICEDiag(Exp->IK, E->getBeginLoc()); */
     switch (Exp->getOpcode()) {
----------------
remove


================
Comment at: clang/lib/AST/ExprConstant.cpp:15750
     return EvaluateCPlusPlus11IntegralConstantExpr(Ctx, this, nullptr, Loc);
-
   ICEDiag D = CheckICE(this, Ctx);
----------------
undo


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131416