[PATCH] D61051: [analyzer] Treat functions without runtime branches as "small".

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Nice! I have no objections (other than the nits) to this! Ill take a second look later, because I really need to play around with `CFG` a bit to give a definitive LGTM.



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:723
+  /// should always inline simply because it's small enough.
+  bool isSmall(AnalysisDeclContext *ADC) const;
+
----------------
Your explanation of state splitting, and early returns and the like could be added here as well :)


================
Comment at: clang/lib/Analysis/CFG.cpp:4684
+  if (size() <= 3)
+    return true;
+
----------------
What are the cases for the size being 2 or 1? Empty function? Is a size of 1 even possible? Can we enforce something here with asserts?


================
Comment at: clang/lib/Analysis/CFG.cpp:4686
+
+  // Traverse the CFG until we find a branch. TODO: While this should still be
+  // very fast, maybe we should cache the answer.
----------------
Break TODO to new line.


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:890
   // Do not inline large functions.
-  if (CalleeCFG->getNumBlockIDs() > Opts.MaxInlinableSize)
+  if (CalleeCFG->getNumBlockIDs() >= Opts.MaxInlinableSize)
     return false;
----------------
Nice.


================
Comment at: clang/test/Analysis/inline-if-constexpr.cpp:16
+void bar() {
+  clang_analyzer_eval(f7()); // expected-warning{{TRUE}}
+}
----------------
Aha, we wouldve seen UNKNOWN because the analyzer wouldn't inline these many functions, right? Neat!


================
Comment at: clang/unittests/Analysis/CFGTest.cpp:117
+  expectLinear(true,  "void foo() { do {} while (false); }");
+  expectLinear(true,  "void foo() { foo(); }"); // Not our problem.
 }
----------------
What do you mean?


Repository:
  rC Clang

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

https://reviews.llvm.org/D61051