rsmith added inline comments.

================
Comment at: clang/lib/AST/ExprConstant.cpp:4583
   if (!InitE)
     return getDefaultInitValue(VD->getType(), Val);
 
----------------
The initializer might also be null because the variable is type-dependent (eg, 
`X<contains_errors> x;`), in which case assuming default-init is wrong. We 
should check for that and treat it like a value-dependent initializer.


================
Comment at: clang/lib/AST/ExprConstant.cpp:4961
     }
+    if (IS->getCond()->isValueDependent())
+      return EvaluateDependentExpr(IS->getCond(), Info);
----------------
hokein wrote:
> The `if` stmt (the same to `while`, `for`) is tricky -- if the condition is 
> value-dependent, then we don't know which branch we should run into.
> 
> - returning a ESR_Succeeded may lead to a bogus diagnostic ("reach the end of 
> the function");
> - returning a ESR_Failed may lead to a bogus diagnostic ("never produce a 
> constexpr"); 
> 
> I guess what we want is to stop the evaluation, and indicate that we hit a 
> value-dependent expression and we don't know how to evaluate it, but still 
> treat the constexpr function as potential constexpr (but no extra diagnostics 
> being emitted), but the current `EvalStmtResult` is not sufficient, maybe we 
> need a new enum.
We should only produce the "never produce a constant expression" diagnostic if 
we also produce a CCEDiag/FFDiag, so I think returning ESR_Failed here should 
work.


================
Comment at: clang/lib/AST/ExprConstant.cpp:4961
     }
+    if (IS->getCond()->isValueDependent())
+      return EvaluateDependentExpr(IS->getCond(), Info);
----------------
rsmith wrote:
> hokein wrote:
> > The `if` stmt (the same to `while`, `for`) is tricky -- if the condition is 
> > value-dependent, then we don't know which branch we should run into.
> > 
> > - returning a ESR_Succeeded may lead to a bogus diagnostic ("reach the end 
> > of the function");
> > - returning a ESR_Failed may lead to a bogus diagnostic ("never produce a 
> > constexpr"); 
> > 
> > I guess what we want is to stop the evaluation, and indicate that we hit a 
> > value-dependent expression and we don't know how to evaluate it, but still 
> > treat the constexpr function as potential constexpr (but no extra 
> > diagnostics being emitted), but the current `EvalStmtResult` is not 
> > sufficient, maybe we need a new enum.
> We should only produce the "never produce a constant expression" diagnostic 
> if we also produce a CCEDiag/FFDiag, so I think returning ESR_Failed here 
> should work.
Should this check live in EvaluateCond instead?


================
Comment at: clang/lib/AST/ExprConstant.cpp:5053
         FullExpressionRAII IncScope(Info);
         if (!EvaluateIgnoredValue(Info, FS->getInc()) || !IncScope.destroy())
           return ESR_Failed;
----------------
Missing value dependence check here.


================
Comment at: clang/lib/AST/ExprConstant.cpp:7896
   QualType Type = Inner->getType();
-
+  if (Inner->isValueDependent())
+    return EvaluateDependentExpr(Inner, Info);
----------------
How does this happen? I would expect the dependence of the subexpression to be 
reflected in the MaterializeTemporaryExpr.


================
Comment at: clang/lib/AST/ExprConstant.cpp:8440
     } else {
+      if (SubExpr->isValueDependent())
+        return EvaluateDependentExpr(SubExpr, Info);
----------------
How does this happen?


================
Comment at: clang/lib/AST/ExprConstant.cpp:9145
   } else if (Init) {
+    if (Init->isValueDependent())
+      return EvaluateDependentExpr(Init, Info);
----------------
How does this happen? Do we not propagate value-dependence from initializers to 
new-expressions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84637

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to