[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.


================
Comment at: test/Analysis/diagnostics/macros.cpp:33
 void testDoubleMacro(double d) {
-  if (d == DBL_MAX) { // expected-note {{Taking true branch}}
+  if (d == DBL_MAX) { // expected-note {{Assuming 'd' is equal to DBL_MAX}}
+                      // expected-note at -1 {{Taking true branch}}
----------------
NoQ wrote:
> This one's good. Static Analyzer doesn't understand floats, so this branch is indeed non-trivial. There should indeed be an assuming... piece here.
A little bit misunderstandable, because we do not know anything about doubles. May the generic message fit better?


================
Comment at: test/Analysis/diagnostics/undef-value-param.m:56
     SCDynamicStoreRef ref = anotherCreateRef(&err, x);
-    if (err) { 
-               //expected-note at -1{{Assuming 'err' is not equal to 0}}
-               //expected-note at -2{{Taking true branch}}
-        CFRelease(ref);
-        ref = 0; // expected-note{{nil object reference stored to 'ref'}}
+    if (err) { //expected-note{{Taking true branch}}
+      CFRelease(ref);
----------------
I am not sure why but the range here [1, (2^64)-1]. There is would not be any number like 0?


================
Comment at: test/Analysis/new-ctor-malloc.cpp:11
   void *x = malloc(size); // expected-note {{Memory is allocated}}
-  if (!x) // expected-note    {{Assuming 'x' is non-null}}
-          // expected-note at -1 {{Taking false branch}}
+  if (!x) // expected-note {{Taking false branch}}
     return nullptr;
----------------
The range information is [1, (2^64)-1] but as I saw `malloc` could return null. Who is lying here?


================
Comment at: test/Analysis/uninit-vals.m:167
   testObj->origin = makePoint(0.0, 0.0);
-  if (testObj->size > 0) { ; } // expected-note{{Taking false branch}}
+  if (testObj->size > 0) { ; } // expected-note{{Assuming the condition is false}}
+                               // expected-note at -1{{Taking false branch}}
----------------
NoQ wrote:
> These are pretty weird. As far as i understand the test, these should be there. But i'm suddenly unable to debug why were they not shown before, because there's either something wrong with exploded graph dumps or with the exploded graph itself; it appears to be missing an edge right after `size > 0` is assumed. I'll look more into those.
We gather information with `clang-analyzer-eval` functions so the conditions would be `Knowing...` pieces but the `BugReporter.cpp` does not know about these extra assumptations. Where to teach to do not `Assuming...`, but `Knowing...` these?


================
Comment at: test/Analysis/virtualcall.cpp:172
 #endif
       X x(i - 1);
 #if !PUREONLY
----------------
Because this `X x(i - 1);` we assume these conditions? This one is tricky to be `Knowing...`.


https://reviews.llvm.org/D53076