mboehme accepted this revision.
mboehme added inline comments.

================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:113
+  // Don't do anything special for CXXForRangeStmt, because the condition 
(being
+  // implicitly generated) isn't visible from the loop body.
+
----------------
Making this purely a comment might risk getting it attached to the wrong method.

Consider adding an empty `VisitCXXForRangeStmt()`, which would make the "do 
nothing" behavior explicit in code?


================
Comment at: 
clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp:1631
+         const AnalysisOutputs &) {
+        EXPECT_THAT(Results.keys(), UnorderedElementsAre("after_loop"));
+      });
----------------
Is this check (and the `(void)0` with the `[[after_loop]` annotation) actually 
needed?

IIUC, the condition we want to test is that the analysis converges. The fact 
that it produces a `Results` entry for `[[after_loop]]` is really more a check 
of the testing infrastructure rather than the analysis itself?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158848

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

Reply via email to