In D63279#1590676, @NoQ wrote:

/me has just noticed that this isn't D34812.

In D64287#1589941, @xazax.hun wrote:

LGTM!

Since we allow new kinds of SVals to be tracked it would be great to test this first on a larger corpus of projects just to see if there is a crash (due to an unhandled SVal type).

Rebase on top master. Putting this on the bottom of the patch stack because this really deserves it's own analysis. (Side note, I completely messed up like ~40 hrs worth of analysis because I didn't check which branches do I have stacked on top of each other, so this might take a while...)

I think you forgot to remove /* */ and clang formatting before uploading the patch.

Hmm, okay, so we convert -1 from the config file to UINT_MAX in the code, I like it!

Starting to look real good!

In D64454#1587102, @aaron.ballman wrote:

I think this looks reasonable to me, though I am still not certain if the relative path in the python script will work with both the svn in-tree directory layout as well as the git monorepo layout (which I'm far less familiar with).

Also, shouldn't we add this to the release notes? In general, it's be around time to sort it out (might do that myself before the new branch).

I don't see obvious red flags strictly regarding the analyzer!

In D64274#1574086, @NoQ wrote:

Hmm, wait, i don't really break backwards compatibility. Fridays...

In D64271#1576872, @NoQ wrote:

I'd rather not abandon this patch, because it looks like a strict improvement over the lack of condition tracking, and it might as well still be an improvement over "zealous" condition tracking, as my counterexample is fairly artificial. It indicates that a slightly more sophisticated algorithm is necessary (i'm not sure if it's single-pass or even linear). But i'll be perfectly happy with simply adding it as a FIXME test.

Gentle ping.

Gentle ping.

Would you say it's good to go? :)

Hmmm, did this result in an assertion?

LGTM! Thanks!

LGTM!

Woohoo!

Np, please leave it in! :)

I guess any time we modify analyzer stuff, we may invite the main analyzer developers to the patch review as well.

Please know that I'm currently out of town, so it'll be a while before I can formally accept. Its on top of my list when I get home though! :^)

Just thinking aloud!

In D64274#1574118, @NoQ wrote:

Mmm, no, not really; it seems that if i introduce a checker dependency, i also have to put the option onto the base checker, otherwise the checker name wouldn't match when i do getCheckerBooleanOption(getChecker<VirtualCallChecker>(), "PureOnly"). Which means that the option name will inevitably change. @Szelethus, do i understand this correctly?

I happen to have very recent analyses on a couple projects, I'll throw this in: LLVM+Clang+Clang-tools-extra. No findings on Xerces or Bitcoin.

Rebase.

Rebase after D64271 being abandoned.

You're right. If condition tracking only adds necessary information anyways, this shouldn't hurt that much anyways.

In D64232#1570938, @NoQ wrote:

I'm slightly worried that we're fighting the symptoms rather than the root cause here: why were these values tracked that far in the first place when we already have no interest in tracking them at the end of the function?

Could you please elaborate? Which of the modified test cases (or any other) do you think falls under "being tracked too far" and why? Whenever the CFG where the value isn't linear, I think the information could be valuable, see the inline.

I.e., i suspect that your "mild tracking mode" would get rid of a lot of those automagically.

In D63538#1571451, @Szelethus wrote:

Since the followup patches test this roughly anyways, and the fact that the AST's lifetime ends right after the CFG's construction makes the remaining tests pretty much pointless, if I can't resolve this, I'll just remove the testfile.

In D63538#1571418, @RKSimon wrote:

@Szelethus This is causing problems on windows buildbots http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast - revert?

• Add two more test cases when a "Returning value" note is meaningful, and one where it's not
• Fix inlines!

LGTM

This checker isn't in alpha -- did you evaluate it on LLVM? Other than that, looks great!

Add one more assert to GetExprText.

• Bail out if the actual terminator isn't a branch
• Bail out if the number of successors is less than 2
• LLVM-ify the code as suggested!
• Add some unit tests (I mean, you can kinda see how it was duct taped together, but it's maybe a hair better than nothing?)

BRILLIANT

Let's not try to tinker with something in a way that could have unforeseen consequences. I added a new method to simply get the condition the way I (and probably @xazax.hun) will need it.