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