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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso marked 2 inline comments as done.
Charusso added a comment.

@george.karpenkov thanks you for the comments!

In D53076#1308641 <https://reviews.llvm.org/D53076#1308641>, @george.karpenkov wrote:

> What about just dropping the `Knowing` prefix?
>  Just having `arr is null, taking true branch` seems considerably more readable.


I wanted to create something identical to the current reports. If we are about to change readability, I would change that two first:

- identical operator printing with `<, <=, >...`, so `equal to` is `=` and `not equal to` is `!=`.
- emphasize the value information with quotes, so `Assuming 'i' >= '2'`.



================
Comment at: test/Analysis/uninit-vals.m:324
   testObj->origin = makeIntPoint2D(1, 2);
-  if (testObj->size > 0) { ; } // expected-note{{Taking false branch}}
+  if (testObj->size > 0) { ; } // expected-note{{Assuming 'testObj->size' is <= 0}}
                                // expected-note at -1{{Taking false branch}}
----------------
george.karpenkov wrote:
> That does not seem right: from `calloc` the analyzer should know that the `testObj->size` is actually zero.
That `Assuming...` piece is rely on the change between the last two diffs: I just print out more `VarDecls` in `patternMatch()`. I thought everything works fine, so I will check that problem if @NoQ will not be faster as he already found some problematic code piece in that test file.


================
Comment at: test/Analysis/virtualcall.cpp:170
+	// expected-note-re at -4 {{{{^}}Assuming 'i' is <= 0}}
+	// expected-note-re at -5 {{{{^}}Taking false branch}}
 #endif
----------------
george.karpenkov wrote:
> Could you describe what happens here?
> Why the `assuming` notes weren't there before?
We do not have range information in the *newly seen `Assuming...` pieces. Because the analyzer has not learnt new information - as there is no information - we have not entered to `VisitTrueTest()` to report these. A good reference for that new behaviour is in `test/Analysis/Inputs/expected-plists/cxx-for-range.cpp.plist`.

*I have added these with https://reviews.llvm.org/D53076?id=175177


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

https://reviews.llvm.org/D53076