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

Fri Oct 19 15:46:45 PDT 2018

rsmith added inline comments.

================
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
----------------
jyknight wrote:
> rsmith wrote:
> > 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.
> I'm not really sure what you mean to distinguish between C/C++98 "evaluation constant expression rules" and the definition of a C/C++98 ICE?
> 
> Perhaps you mean that if evaluation succeeds, it will have produced a value consistent with the rules of the current language, but that it may not produce the diagnostics in all cases it ought to?
> 
> AFAICT, the evaluator is designed to emit diagnostics only for the C++11 (and later) constant expression rules, as well as for inability to evaluate. The diagnostic notes are usually dropped on the floor in C++98 and C modes, as only "fold" is typically used there -- along with the separate CheckICE to produce warnings.
> 
> I believe the only place the diagnostics were being actually emitted in pre-c++11 modes is for the diagnose_if and enable_if attributes' "potential constant expression" check. And there it really seems to me to be using the c++11 evaluation semantics.
> 
> The existence of isCXX11ConstantExpr suggests an intent for the function to provide the C++11 rules, as well, no?
> 
> 
> Perhaps you mean that if evaluation succeeds, it will have produced a value consistent with the rules of the current language, but that it may not produce the diagnostics in all cases it ought to?

The concern I have with this comment is that it describes a happenstance of implementation rather than the intent. The intent is certainly *not* that we will use the C++11 rules when calling this in C++98 mode, but it just so happens that there are no interesting differences there right now.

Looking at this another way, the fact that we only guarantee to produce "not a constant expression because" notes in C++11 onwards is unrelated to the evaluation mode. Rather, the reason for that is instead because all the validity / ICE checking in other language modes is based on the syntactic form of the expression (and is checked separately as a result).

As a concrete suggestion, how about something like:

"Evaluate as a constant expression. Stop if we find that the expression is not a constant expression. Note that this doesn't enforce the syntactic ICE requirements of C and C++98."

================
Comment at: clang/lib/AST/ExprConstant.cpp:952
+    /// with diagnostics.
+    bool keepEvaluatingAfterNote() {
       switch (EvalMode) {
----------------
jyknight wrote:
> rsmith wrote:
> > "Note" is beside the point here. We should be talking about non-constant expressions; that's what matters.
> Renamed to keepEvaluatingAfterNonConstexpr, updated comment.
Capitalize as "...NonConstExpr" instead, since this is just an abbreviation of the term "constant expression", not C++11's notion of "constexpr". (I'd also lengthen this to "...NonConstantExpr" or "...NonConstantExpression" to further clarify.)

================
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();
----------------
jyknight wrote:
> rsmith wrote:
> > `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.)
> Are you sure?
> 
> Today, checkSubobject returns false if it sees something "invalid" -- and the callers seem to depend on that -- but invalid subobject designators are still allowed during evaluation in fold mode.
> 
> E.g., this is valid in C:
> `struct Foo { int x[]; }; struct Foo f; int *g = f.x;`
> 
> But, in C++, `constexpr int *g = f.x;` is an error, diagnosing note_constexpr_unsupported_unsized_array.
> 
> All the LValue code, with its mutable "Result" state, and carrying of a couple different "Invalid" bits off on the side, is really rather confusing!
What I mean is that you should change the code to make what I said be true, if necessary. It's the callee's responsibility to say whether we hit something we can't evaluate past. The caller should not be guessing that the only reason that `checkSubobject` can fail is due to a `CCEDiag` -- if `checkSubobject` fails with an `FFDiag`, this will sometimes return `true`, which would be wrong.

Here's how I'd look at it: a `false` value is a token that indicates that someone has produced a diagnostic whose severity is such that we don't need to keep evaluating. Once that happens, we don't need to re-check whether to keep evaluating, because we've already been told that we shouldn't, and we just need to propagate that fact to our caller.

================
Comment at: clang/lib/AST/ExprConstant.cpp:710
+      // EM_PotentialConstantExpressionUnevaluated as a duplicate).  This will
+      // be somewhat tricky to change, because LLVM currently verifies that an
+      // expression is a constant expression (possibly strictly with
----------------
LLVM -> Clang (or "we" or similar)

================
Comment at: clang/lib/AST/ExprConstant.cpp:723
+      /// Fold the expression to a constant, using any techniques we can. Stop
+      /// if we hit a side-effect that we can't model, or undefined behavior.
       EM_ConstantFold,
----------------
Do we now guarantee to stop on UB in this mode? We didn't appear to do so previously. (Particularly, we'd evaluate past signed overflow and take the 2's complement value.)

================
Comment at: clang/lib/AST/ExprConstant.cpp:956
+    /// expression which is not a constant expression? If this returns true,
+    /// then the evaluation will be successful if and only no diagnostics are
+    /// emitted. If it returns false, evaluation can succeed with diagnostics.
----------------
only no -> only if no

================
Comment at: clang/lib/AST/ExprConstant.cpp:1198-1200
+  if (!Info.keepEvaluatingAfterNonConstexpr())
+    return false;
+  return true;
----------------
Simplify to `return Info.keepEvaluating...();`

================
Comment at: clang/lib/AST/ExprConstant.cpp:2527-2529
+  if (!LVal.adjustOffsetAndIndex(Info, E, Adjustment, SizeOfPointee))
+    return false;
   return true;
----------------
Simplify to `return LVal.adjust[...]`

================
Comment at: clang/lib/AST/ExprConstant.cpp:7438
+      return VisitIgnoredBaseExpression(E->getBase());
     }

----------------
Nit: drop the braces here.

================
Comment at: clang/lib/AST/ExprConstant.cpp:11490-11492
+  // _is_ a constant expression.  (Note: the above applies in
+  // EM_ConstantExpression mode, which we use here, but not in,
+  // e.g. EM_ConstantFold or others.)
----------------
I don't believe the parenthetical is true: in C++11 or later, we should always get a note if and only if the expression is not a constant expression, regardless of evaluation mode. Instead, I think the thing you're highlighting here is that the return value from `EvaluateAsRValue` does not indicate whether the expression is a constant expression unless the evaluation mode is `EM_ConstantExpression`.

================
Comment at: clang/lib/AST/ExprConstant.cpp:11495-11496
+
   if (!Diags.empty()) {
-    IsConstExpr = false;
     if (Loc) *Loc = Diags[0].first;
   }
----------------
Combine these two `if`s.

https://reviews.llvm.org/D52939