Thanks!!

Yup, optional sounds perfect.

In D67079#1687577, @Charusso wrote:

• CastVisitor is the new facility which decides whether the assumptions are appropriate.
• The math is still WIP.

I actually like the idea, it makes it consistent with other maps. But you'll need to clean up memory management here. Given that you can't modify the state in getDynamicTypeInfo(), i guess you'll have to resort to smart pointers.

Fantastic, thanks!!

Thank you, fantastic finding!

+@Szelethus because i'm a bit out-of-the-loop on plugins: i very vaguely remember that we decided to put them into tests(?)

In D67706#1685963, @Szelethus wrote:

It seems like this patch is diffed against your latest commit, not the master branch.

Yeah, i think we should avoid such peeking and instead try to do everything in one pass. I.e., if we need to peek at the node above us, just make a visitor that delays the decision until it has precisely the information it needs. I guess i'll be slooowly moving in this direction.

Thanks! I like how it makes the option transparent to fellow developers by having it show up in the -### run-line.

Burn it!

In D67682#1673332, @arphaman wrote:

Can we always pass in -D from the driver instead?

Accept as soon as Alex's comment is addressed somehow :)

Is it just me or phabricator somehow refuses to display the changes since the last diff here? That's probably the commit diff is involved somehow. So i'm confused what exactly has changed >.<

You can go one step further to have a CallDescriptionMap, like in D62557. This would replace the whole chain of ifs with a map lookup (which is currently still a chain of ifs under the hood, but a lot less code anyway and we'll probably optimize it under the hood later).

Btw, evalCall is not deprecated. In fact, there are no alternatives for it in many cases.

That's a nice twist, i like it!

FTR, we already have a similar Static Analyzer check, eg.: https://github.com/llvm-mirror/clang/blob/release_80/test/Analysis/dispatch-once.m#L15

rC371781.

Umm, this test fails for me locally because i see osx checkers displayed in the list. Which is weird because they're supposed to be enabled based on the target platform, not on the host platform.

SimpleStreamChecker is a historical tutorial example, i don't think we should be updating it other than for modernizing checker API use. It was supposed to handle a few simple examples correctly but it wasn't supposed to support all sorts of different APIs.

rL371773 - thanks @timshen!

In D67420#1668099, @Szelethus wrote:

...and the second is similar in nature, but in the actual code -- it doesn't doesn't feel natural to me that AnalyzerOptions is required to construct this, while at the same time we're trying to make diagnostics construction independent of the analyzer.

I'd like the test cases to actually demonstrate the correct use of the filters and the correct behavior of the Analyzer when the filters are annotated correctly, but it looks to me that they either demonstrate behavior when the annotation is not used correctly, or we disagree about how the taint should work in the first place. Testing the behavior when the annotation is misplaced is fine (with enough comments and probably FIXMEs where applicable), but "positive" tests are more valuable because they are the actual common cases (hopefully).

Aha, nice, thank you!!

Unforget to actually move the consumer implementations to libAnalysis, not just their headers.

Clean up a tiny bit of dead code.

Rename BugType to WarningMessage, add comments.

Unforget to do the same for the text consumer. As a side effect, the factory function for the text consumer is no longer special, which will be less confusing when put in libAnalysis.

In D67419#1665993, @Szelethus wrote:

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

In D67381#1665930, @Szelethus wrote:

Side note, now that you had to work with the freshly rewritten file, do you have any feedback on it?

Hmm, does anybody want me to write an example tool that emits path diagnostics but doesn't link to libStaticAnalyzer*?

In D67420#1666050, @gribozavr wrote:

Are you planning to move users of these options -- like PlistDiagnostics -- to libAnalysis?

In D67420#1666141, @Szelethus wrote:

• For these select 4 options that suffer compatibility issues, manually check AnalyzerOptions::ConfigTable.

In D67420#1666141, @Szelethus wrote:

I do!

In D67421#1666058, @gribozavr wrote:

The "bugtype" in IssueHash is specific to Static Analyzer (or at least not documented sufficiently abstractly).

In D67140#1664406, @gribozavr wrote:

%select

I have mixed feelings. Removing boilerplate is good, but the very fact that we're legalizing this pattern indicates that our checkers will keep bloating up, while i always wanted to actually split them instead (like, make sub-checkers into their own separate classes, possibly spread out into different files, kinda micro checkers as opposed to monolithic checkers (?)). But i guess it's about whoever gets things done first :)

In D64991#1662185, @Szelethus wrote:

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
}

In D67140#1659982, @gribozavr wrote:

We should take a page from desktop software here. If the messages were in a separate file, there would be a lot of people capable of mass-editing them. When messages are hardcoded in the tool code, navigating and editing them requires more skill, and definitely a lot more jumping around.

In D67156#1661881, @Szelethus wrote:

I'm sadly not knowledgeable enough with CallDescriptionMap

Yup, thanks, this is really nice!

Just rebase.

Fxd, thanks!

Fix most comments.

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

In D67140#1659774, @aaron.ballman wrote:

I don't think it's a requirement (so long as the diagnostics are clear about the issue being diagnosed, I'm happy enough), but I think it's good for a tool to be self-consistent in its messaging. It's jarring that one part of the compiler emits diagnostics one way and another emits them a totally different way. It may be less of an impact for people who don't see the output from both at the same time, but that happens to be the use case I have.

You don't have commit access, right? Do you want me to commit this patch for you?

I wish i had time to fix D55683 so that i could have you write tests for it :)

Yay thanks!!

Make fix-it hints attachable to any PathDiagnosticPiece, rather than to PathDiagnostic as a whole. This basically allows attaching a fixit to any note in the report. For now the plist consumer only supports attaching fixits to the warning itself, to path event pieces, and to extra note pieces; it's completely unclear what's the benefit of attaching fixits to other kinds of pieces.