Szelethus requested changes to this revision.
Szelethus added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/StaticAnalyzer/Core/LoopUnrolling.cpp:214-216
+  const Expr *BoundExpr = CondOp->getLHS()->IgnoreParenImpCasts();
+  if (BoundExpr == Matches[0].getNodeAs<Expr>("boundVarOperand"))
+    BoundExpr = CondOp->getRHS()->IgnoreParenImpCasts();
----------------
NoQ wrote:
> Szelethus wrote:
> > Alright, I stood on top of this for longer than I'd like to admit, what's 
> > happening here? Like, `CondExpr`, with the given testfile, should be `i < 
> > n`, right? Then `BoundExpr` is supposed to be `i`, and if that is equal to 
> > `Matches[0].getNodeAs<Expr>("boundVarOperand")` (which I guess is supposed 
> > to `n`), which I'm not sure how it could ever happen, we assign the RHS of 
> > the assignment to `BoundExpr`?
> > 
> > What does this do? Why is it needed? Which 2 test cases cover these lines?
> I think this is about `i < n` vs. `n > i`.
If that is the case, I'd be more comfortable seeing a test for it (there 
doesn't seem to be any) before landing this.


================
Comment at: lib/StaticAnalyzer/Core/LoopUnrolling.cpp:221-230
+  if (BoundNumVal.isUnknown()) {
+    if (const auto *BoundDeclRefExpr = dyn_cast<DeclRefExpr>(BoundExpr)) {
+      // FIXME: Add other declarations such as Objective-C fields
+      if (const auto *BoundVarDecl =
+              dyn_cast<VarDecl>(BoundDeclRefExpr->getDecl())) {
+        BoundNumVal = State->getSVal(
+            State->getLValue(BoundVarDecl, Pred->getLocationContext()));
----------------
NoQ wrote:
> Szelethus wrote:
> > I don't see obvious test case for which `BoundNumVal` would be unknown, am 
> > I wrong?
> We need an `ExprInspection` method that reliably produces an `UnknownVal`, 
> because there's no truly valid reason to produce `UnknownVal` apart from 
> "something is unimplemented".
For this too.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63279



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

Reply via email to