While we're there, could you please see whether the included test case (note how condition tracking is turned off) fails on your platform without the rest of the patch applied (it definitely does on mine, which is why I was surprised to see this bug report pop up only now)? If not, I can just push a small commit with the llvm::isa_and_nonnull change to get some breathing room.

In D66716#1671105, @TWeaver wrote:

Hi there,

just checking in, is this patch still going ahead?

thanks
Tom W

Aha, so every user gets to create their own PathDiagnosticConsumerOptions object, makes sense! There is no interface misconception, because -analyzer-config will only configure what the analyzer would tinket with. I like this patch! If you dont mind, I'd prefer to have one last round with this, but otherwise LGTM.

Yes, thank you! I've been keeping my mailbox open and commiting slowly, it seems like the buildbots have a wrong email address set up for me.

In D67420#1668474, @NoQ wrote:

It's not that AnalyzerOptions is necessary to construct this, it's more like sufficient.

In D67420#1666578, @NoQ wrote:

In D67420#1666141, @Szelethus wrote:

I do!

Hmm, it sounds like you want to force both Clang frontend and Clang-Tidy to use the same set of flags to control these options (?) Much like -analyzer-config, these flags will have different "style" compared to other flags in the tool. Which is probably not too bad for hidden frontend flags that control the Analyzer because they're anyway set by a separate GUI checkbox, but the inconsistency with other flags would be super glaring in case of Clang-Tidy CLI. Do we really want to go in that direction? I'll be much more comfortable with letting each tool deal with its flags the way it prefers - this doesn't look like a good place for code reuse to me.

@Szelethus: I could have stored PathDiagnosticConsumerOptions in AnalyzerOptions by value and pass a const reference around, but it wasn't pleasant to integrate with AnalyzerOptions.def. I.e., i'd have to implement a new kind of option that doesn't allocate its own field but instead re-uses a field within a sub-object. Do you want me to go for it or is this implementation good enough?

Looks great! Are we sure that PathDiagnostic.h is a good header name?

LGTM! I think code readability improved geatly.

I'm open to discuss a better design here. Eg., i thought about making it part of the visitor interface instead, but i don't immediately see how to do this without breaking the logic of "only add the note at the call site in which the event has happened, not every time allocated memory is returned from anywhere".

Sure!

Hmm, we could make a redundant assignments checker: if a variable has multiple reaching definitions, but those all assign the same value, emit a warning. We could even use fixits with that.

void t(int a) {
  if (coin())
    a = 2; // note: reaching def
  else
    a = 2; // note: reaching def
  use(a); // warn: a is always 2 here
}

I'm sadly not knowledgeable enough with CallDescriptionMap, so let's have another round of review on this, otherwise, its perfect.

Niiiiice! Thank you so much for sorting this out! I think this will make reviewing far more accessible for newcomers to the IteratorChecker family. I have a couple nits to comment on, but I won't clutter the code just yet. @NoQ, do you have any high level objections to this?

But, of course, let's wait for @gribozavr to give his blessings as well, I'm only accepting because removing/changing other parts of the API seems to deserve a separate revision :)

This patch set the goal of splitting BugReport into two different report kinds, and I think it did that well. Not only that, we drastically improved the documentation, formalized many things that weren't in the core before, so I wouldn't like you to bear the burdern of never ending rebases (it doesn't make reviewing easier either). Let's work on the rest of the code in a followup patch.

I'm fine with moving in-class function into anonymous namespace later. LGTM, feel free to commit!

In D67140#1659907, @NoQ wrote:

In D67140#1659872, @aaron.ballman wrote:

If we're okay with the inconsistency but still feel like giving ourselves more work, adding proper punctuation to Static Analyzer diagnostics would at least make them grammatically correct. :-D

I'd love us some punctuation! Unfortunately the last time a person who actually speaks English committed actively to the Static Analyzer was, like, a couple of years ago at least (:

I don't have strong feelings on capitalization itself either, but the idea of longer, multi-sentence bug report messages (maybe this could finally be used? D66572#inline-602143) sound pretty cool.

I'm still working on this, just been kinda busy. I'll try to get it out of the way asap.

In D67140#1658365, @aaron.ballman wrote:

Then again, with the recent resurfacing of discussions about renaming everything under the sun, maybe we've changed our community opinion here. :-D I guess I don't see Check vs Checker to be worthy of breaking everyone's out-of-tree code over.

Also, please mark inlines done as you fix them :)

Some nits inline, otherwise LGTM. @steakhal, do you have anything to add to this?

@gribozavr, @alexfh this isn't a particularly exciting patch, but it does highlight a fundamental difference in between the analyzer and clang-tidy, so I added you for the sake of it, since we're talking about a closer interaction anyway :)

Also, thank you @gribozavr for your comments -- its very nice to have someone review a bigger part of our development interface who is knowledgeable about Clang, but not the Static Analyzer specifically. Looking at your inlines, these are very fair criticisms, I found (find) these confusing when I just started contributing as well.

Other then the things @gribozavr mentioned, LGTM.

In D66042#1655119, @sylvestre.ledru wrote:

@Charusso This probably should be added to the release notes:
https://clang.llvm.org/docs/ReleaseNotes.html#static-analyzer
and detailed in the doc.
Please let me know if you need help!

Sorry for not accepting this -- I actually read the code multiple times and didn't see anything that stood out! I didn't have the time to dig too deep, but if the tests are anything to go by, its gotta be alright.

Excellent detective work! Thanks! I'll do the honors.

In D66733#1647964, @steakhal wrote:

@Szelethus The mispositioned report message was my fault. I used a different version of clang for the analysis and to upload the results, which resulted in some mispositioned reports.
I've fixed the linked CodeChecker instance.

In D66765#1646237, @NoQ wrote:

I approve the patch and i don't see anything obvious that we're missing out (@Szelethus, your GSoC isn't on by default back in 9.0, only in master, right?).

Fixing inlines!

Another common mistake is this:

if (A) {
  const auto *B = dyn_cast_or_null<FunctionDecl>(A); // warn: A is known to be non-null, prefer dyn_cast
}

if the dyn_ part is really necessary here, then you crash with a null dereference a few lines below because you didn't check the result

In D66716#1647668, @NoQ wrote:

I don't understand. Isn't widening supposed to happen after we exit the loop? The loop isn't over yet when the bug is being reported. Why is this problem widening-specific? Given that we also have a weird invalidation of b, i suspect that we're doing widening in a wrong moment of time.

Note how I am partially at fault here, which is why the bug reports state that the crash originates from TrackControlDependencyVisitor, but even after that is fixed, the diagnostics construction flopped on an out-of-bounds error.

In D66404#1645851, @comex wrote:

Heh, I guess I'll request commit access, although I'm not sure if I have enough of a 'track record of submitting high quality patches'.

Please note that LLVM 9.0.0-final is due on the 28th of August.

(++LGTM)++

I really-really like this change. The results look great, though I'm not sure what happened here, are we sure that this originates from the analyzer, and is not some storing error?

In D66721#1644514, @NoQ wrote:

No-no, you're disabling the checkers here but you should be silencing them. This will be crashing because modeling is disabled.

I also recommend checker options, even if they apply to multiple checkers (@Szelethus mentioned that package options are a thing, you might use these if you don't want to make multiple options).

Mind that I'll probably commit without the unit test, last time I learned the hard way that the AST's lifetime ends by the time we retrieve the CFG.

No worries, always happy to help! :)

I seem to have been able to put this together by fetching the individual diffs and squashing them this time :)

What I meant is that the diff has to be made against the master branch or I won't be able to apply it locally. Say your repo is structured like this:

@whisperity, any objections to this?

You got me convinced.

Super high level question: CheckerManager knows whether a checker, and I suspect a checker callback is path sensitive or not, do you think we can automate this decision (whether the bug report is path sensitive of syntactic)? For the purposes of clang-tidy, can we say such a thing that if we can't prove a bug report to be path sensitive, it will not be that?