[PATCH] D52939: ExprConstant: Propagate correct return values from constant evaluation.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/lib/AST/ExprConstant.cpp:12-32
 // Constant expression evaluation produces four main results:
 //
 //  * A success/failure flag indicating whether constant folding was successful.
 //    This is the 'bool' return value used by most of the code in this file. A
 //    'false' return value indicates that constant folding has failed, and any
 //    appropriate diagnostic has already been produced.
 //
----------------
Is this comment still correct?


================
Comment at: clang/lib/AST/ExprConstant.cpp:682
     enum EvaluationMode {
-      /// Evaluate as a constant expression. Stop if we find that the expression
-      /// is not a constant expression.
+      /* The various EvaluationMode kinds vary in terms of what sorts of
+       * expressions they accept.
----------------
Nit: LLVM style avoids `/*...*/` comments even for long comment blocks like this (https://llvm.org/docs/CodingStandards.html#comment-formatting).


================
Comment at: clang/lib/AST/ExprConstant.cpp:686-687
+         Key for the following table:
+         * D = Diagnostic notes (about non-constexpr, but still evaluable
+               constructs) are OK (keepEvaluatingAfterNote)
+         * S = Failure to evaluate which only occurs in expressions used for
----------------
I think talking about diagnostic notes here distracts from the purpose, which is that this column indicates that we accept constructs that are not permitted by the language mode's constant expression rules.


================
Comment at: clang/lib/AST/ExprConstant.cpp:688-690
+         * S = Failure to evaluate which only occurs in expressions used for
+               side-effect are okay.  (keepEvaluatingAfterSideEffect)
+         * F = Failure to evaluate is okay. (keepEvaluatingAfterFailure)
----------------
"is okay" here is misleading, I think. The point of `keepEvaluatingAfter[...]` is that it's safe to stop evaluating if they return false, because later evaluations can't affect the overall result.


================
Comment at: clang/lib/AST/ExprConstant.cpp:692
+         * E = Eagerly evaluate certain builtins to a value which would normally
+               defer until after optimizations.
+
----------------
defer -> be deferred?


================
Comment at: clang/lib/AST/ExprConstant.cpp:706-707
+
+         TODO: Fix EM_ConstantExpression and EM_PotentialConstantExpression to
+         also eagerly evaluate. (and then delete
+         EM_PotentialConstantExpressionUnevaluated as a duplicate)
----------------
Allowing `EM_ConstantExpression` to evaluate expressions that `EM_ConstantFold` cannot evaluate would violate our invariants. We rely on being able to check up front that an expression is a constant expression and then later just ask for its value, and for the later query to always succeed and return the same value as the earlier one. (This is one of the reasons why I suggested in D52854 that we add an explicit AST representation for constant expressions.)


================
Comment at: clang/lib/AST/ExprConstant.cpp:711
+
+      /// Evaluate as a constant expression, as per C++11-and-later constexpr
+      /// rules. Stop if we find that the expression is not a constant
----------------
This is not the intent. The evaluator uses the rules of the current language mode. However, it doesn't enforce the C and C++98 syntactic restrictions on what can appear in a constant expression, because it turns out to be cleaner to check those separately rather than to interleave them with evaluation.

We should probably document this as evaluating following the evaluation (but not syntactic) constant expression rules of the current language mode.


================
Comment at: clang/lib/AST/ExprConstant.cpp:720-722
+      /// Like EM_ConstantFold, but eagerly attempt to evaluate some builtins
+      /// that'd otherwise fail constant evaluation to defer until after
+      /// optimization.  This affects __builtin_object_size (and in the future
----------------
This sentence is a little hard to read.


================
Comment at: clang/lib/AST/ExprConstant.cpp:891
     /// expression.
     ///
     OptionalDiagnostic CCEDiag(SourceLocation Loc, diag::kind DiagId
----------------
Nit: delete line


================
Comment at: clang/lib/AST/ExprConstant.cpp:952
+    /// with diagnostics.
+    bool keepEvaluatingAfterNote() {
       switch (EvalMode) {
----------------
"Note" is beside the point here. We should be talking about non-constant expressions; that's what matters.


================
Comment at: clang/lib/AST/ExprConstant.cpp:1227-1230
+    diagnosePointerArithmetic(Info, E, N);
+    setInvalid();
+    if (!Info.keepEvaluatingAfterNote())
+      return false;
----------------
I would strongly prefer to keep the emission of the diagnostic and the corresponding `return false` adjacent whenever possible, so I'd prefer that we add a `bool` return value to `diagnosePointerArithmetic`.

(Eventually, we should remove these `bool` return values entirely and replace them with an enum, where the failure value is only ever created by the `...Diag` functions and from places where we can prove that a diagnostic has already been emitted, so that we can easily ensure that every failure path produces a diagnostic first.)


================
Comment at: clang/lib/AST/ExprConstant.cpp:1440-1444
+      if (checkSubobject(Info, E, isa<FieldDecl>(D) ? CSK_Field : CSK_Base)) {
         Designator.addDeclUnchecked(D, Virtual);
+        return true;
+      }
+      return Info.keepEvaluatingAfterNote();
----------------
`checkSubobject` should return `false` if it produced a diagnostic that should cause evaluation to halt; this call to `keepEvaluatingAfterNote` should not be necessary. (And likewise below.)


================
Comment at: clang/lib/AST/ExprConstant.cpp:11484
+  // Diags should always contain a message when the expression is determined to
+  // not be a constexpr, and should always be empty when the result _is_ a
+  // constant expression.  (Note: the above applies in EM_ConstantExpression
----------------
Nit: there's no such thing as "a constexpr". I think you mean "a constant expression".


================
Comment at: clang/lib/AST/ExprConstant.cpp:11487
+  // mode, which we use here, but not in, e.g. EM_ConstantFold or others.)
+  assert(Diags.empty() == IsConstExpr);
+
----------------
How confident are you that all `return false;`s produce an accompanying note? We have had a long tail of situations where such notes are missing from some cases where evaluation fails. (Those were previously QoI bugs but will now result in an assertion failure.)


================
Comment at: clang/lib/AST/ExprConstant.cpp:11599-11600
   Evaluate(ResultScratch, Info, E);
+  // Same as above -- the success return value from Evaluate is not
+  // useful, we just care about diagnoses.
   return Diags.empty();
----------------
I fear that a "Same as above" comment will become stale during refactorings.


https://reviews.llvm.org/D52939