cor3ntin added inline comments.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:1546
def warn_consteval_if_always_true : Warning<
+ "consteval if is always %select{true|false}0 in this context">,
----------------
To be consistent with other tautological warnings
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8821
"operand to 'typeid'">, InGroup<PotentiallyEvaluatedExpression>;
+def warn_is_constant_evaluated_tauto_constexpr : Warning<
+ "'%select{std::is_constant_evaluated|__builtin_is_constant_evaluated}0' will
always evaluate to %select{false|true}1 in this context">,
----------------
================
Comment at: clang/include/clang/Sema/Sema.h:1277
+ /// as PotentiallyEvaluated.
+ RuntimeEvaluated
};
----------------
hazohelet wrote:
> cor3ntin wrote:
> > I think I'd prefer a boolean for that, `ExpressionEvaluationContext`
> > somewhat matches standard definitions.
> > I think we might mostly be able to reuse PotentiallyEvaluatedIfUsed |
> > PotentiallyEvaluated.
> >
> > RuntimeEvaluated is anything that does not have a parent context that is
> > constant evaluated/immediate/unevaluated. Maybe you could try to make a
> > function for that?
> I made it a boolean instead of a new expression evaluation context.
>
> > RuntimeEvaluated is anything that does not have a parent context that is
> > constant evaluated/immediate/unevaluated. Maybe you could try to make a
> > function for that?
> I dont't think it is correct. The definitions body of constexpr-qualified
> function and non-qualified function both push `PotentiallyEvaluated`, so we
> need some way to distinguish them, and this time it is a boolean.
Right, thanks for clarifying, i missed that
================
Comment at: clang/lib/Parse/ParseDecl.cpp:2440
+ S.ExprEvalContexts.back().Context;
+ if ((D.getDeclSpec().getTypeQualifiers() == DeclSpec::TQ_const ||
+ isNonlocalVariable(ThisDecl)) &&
----------------
Why are we looking at whether static vars are const qualified?
================
Comment at: clang/lib/Parse/ParseStmt.cpp:1513-1516
+ EnterExpressionEvaluationContext Consteval(
+ Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated,
+ /*LambdaContextDecl=*/nullptr,
+ Sema::ExpressionEvaluationContextRecord::EK_Other, IsConstexpr);
----------------
hazohelet wrote:
> cor3ntin wrote:
> > This seems wrong, the condition of a if statement is not constant evaluated.
> In this case `/*ShouldEnter=*/IsConstexpr`, so the constant evaluation
> context is pushed only when it is a condition of constexpr if. The condition
> of constexpr if shall be a constant expression as per
> https://eel.is/c++draft/stmt.if#2
I missed that, makes sense!
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:17962-18006
-/// Determine whether the given declaration is a global variable or
-/// static data member.
-static bool isNonlocalVariable(const Decl *D) {
- if (const VarDecl *Var = dyn_cast_or_null<VarDecl>(D))
- return Var->hasGlobalStorage();
-
- return false;
----------------
hazohelet wrote:
> cor3ntin wrote:
> > Can you explain the changes to this file? they seems to affect test cases
> > unrelated to the goal of this PR
> When we push/pop evaluation context here, it behaves synchronously with
> `InitializerScopeRAII`, in ParseDecl.cpp, which makes the evaluation of
> `Actions.AddInitializerToDecl` happen outside of the initializer expression
> evaluation context (e.g. initializer scope popped in 2581 and
> AddInitializerToDecl evaluated in 2592 in ParseDecl.cpp). This is causing a
> bug BEFORE this patch (https://godbolt.org/z/b18jxKT54 and the removed FIXME
> in test/SemaCXX/cxx2a-consteval.cpp) because we were pushing evaluation
> context if the variable `isNonlocalVariable`. This PR separates the handling
> of expression evaluation context of initializers from `InitializerScopeRAII`
> to resolve these bugs.
>
> The arguable changes in diagnostics caused by this are the removals of
> `warn_uninit_self_reference_in_reference_init` against `constexpr int &n =
> n;` (You mentioned) and
> `warn_impcast_integer_precision_constant` against `constexpr signed char a =
> 2*100;`
> Both are diagnosed by calling `Sema::DiagRuntimeBehavior`, and
> `Sema::DiagIfReachable` called by this intentionally avoids emitting
> diagnostics against initializer of constexpr variables.
> (https://github.com/llvm/llvm-project/blob/ab73bd3897b58ecde22ec5eea14b6252f2d69b6e/clang/lib/Sema/SemaExpr.cpp#L20712-L20719)
> They were diagnosed BEFORE this patch because the expression evaluation
> context was outside of the initializer evaluation context, and it emitted
> false positives against cases like `constexpr signed char a = true ? 3 :
> 200;` (https://godbolt.org/z/9z1rer7E3)
> I think if the evaluated result of constexpr variable initializer does not
> fit the variable type it should be diagnosed, but the logic should be added
> to where we handle constexpr variables.
>
Thanks for the detailed explanation.
Separating InitializerScopeRAII from evaluation scope is something i support.
Did you try removing the test in `DiagIfReachable`, see how much breaks?
In any case, there are 2 fixme in that function already, that should cover us.
It might be useful to better describe these changes in the release notes /
commit message
================
Comment at:
libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.segmented.pass.cpp:96
int main(int, char**) {
- if (!std::is_constant_evaluated()) {
- test_containers<std::deque<int>, std::deque<int>>();
----------------
this is a funny one, what's the history of that?
================
Comment at:
libcxx/test/std/utilities/meta/meta.const.eval/is_constant_evaluated.verify.cpp:27
static_assert(!std::is_constant_evaluated(), "");
- // expected-warning@-1 0-1 {{'std::is_constant_evaluated' will always
evaluate to 'true' in a manifestly constant-evaluated expression}}
+ // expected-warning@-1 0-1 {{'std::is_constant_evaluated' will always
evaluate to true in this context}}
#endif
----------------
Mordante wrote:
> hazohelet wrote:
> > philnik wrote:
> > > Mordante wrote:
> > > > Since libc++ support the latest ToT Clang and the last two official
> > > > releases this wont work. The `expected-warning` needs to be a
> > > > `expected-warning-re` that works for both the new and old diagnostic
> > > You can also just shorten it to `'std::is_constant_evaluated' will always
> > > evaluate to`. Seems good enough to me.
> > Thanks!
> I really would like a regex. To me the current message misses an important
> piece of information; the `true` part. I care less about the rest of the
> message, but stripping the `true` means a warning like
> `std::is_constant_evaluated' will always evaluate to FALSE` would be valid
> too.
Agreed with Mordante
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D155064/new/
https://reviews.llvm.org/D155064
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
