In D62899#1551312, @NoQ wrote:

In D62899#1549715, @Szelethus wrote:

Added a proper testfile. The only downside of it is that it doesn't test anything.

Use creduce!

Thank you, and sorry for dragging you through this! At the very least, we got to learn a lot from it :)

I hope you dont mind me changing the revision title -- many of us are subscribed to the "analyzer" tag and like learning about whats changing :)

I really like the change, thanks! Let's settle on the diagnostic message then.

You seem to have diffed against your latest local commit, rather than against trunk (or master, if you use the monorepo). Phabricator isn't smart enough to put two and two together, and only displays the uploaded diff (though one has to admit, its doing a damn good job at that at least!).

In D66267#1632164, @NoQ wrote:

In D66267#1630728, @Szelethus wrote:

Shouldn't we just delete this entire visitor altogether and merge it into ConditionBRVisitor (like, eventually, not right now)? It seems to be a relic of the past.

I'm actually curious about one particular mess that we have here. Namely, there's a visitor that says "assuming..." and there's checker notes when checkers themselves assume something; how can we be sure they don't duplicate each other?

Oh, there is no need for a new differential, you can update this one by clicking on 'update diff' in the right panel.

Yup, you can upload it and the green checkmark will stay. If it doesn't, I'll accept again.

Shouldn't we just delete this entire visitor altogether and merge it into ConditionBRVisitor (like, eventually, not right now)? It seems to be a relic of the past.

LGTM, thanks!

In D60281#1630646, @NoQ wrote:

Thanks!

One more quick question: Who is Husi???

Fixed!

Revised the documentation according to @NoQ's comments. By literally copy pasting it. Like any good programmer should do :^)

In D65182#1630540, @NoQ wrote:

In D65182#1629192, @Szelethus wrote:

Hmm, why the need for checker options? Why not have them by default? If fixits are an experimental feature, maybe we should have a global enable-fixits config. But I don't insist :)

I did this because fixits have a much higher quality standard that warnings (because people often apply them blindly). This means that fixit support is likely to be lagging behind in meeting the quality standards and therefore it's important to allow some (but not necessarily all) on-by-default checkers have alpha fixits.

In D63279#1630491, @NoQ wrote:

the only reason it's not enabled by default yet is because we forgot to do that :(

In D63279#1606063, @baloghadamsoftware wrote:

For me it is all the same whether to continue this patch or taking over Péter's.

I'll gladly add the finishing touches :)

I really like the high level idea proposed by this patch, and the test files make me believe that its correct as well! I'm really not familiar around this part of the code, so if its okay, I'll take my time to do the usual find references, inserting unreachables/asserts to gain a better understanding before the green checkmark.

I agree with @gamesh411 here, would it be much trouble to see how this behaves on a bigger codebase?

I have no authority to accept patches in clang-tidy (though please feel free to add me as a reviewer, I can more easily participate in the discussion!), but in any case, this looks good to me too, thanks!

• Make the generic messages in-class, constexpr fields.
• Provide plenty of test cases for captured lambda variables. In our meeting we had both had a sight of relief, and some still lingering concerns about this, so maybe it deserves fixing in the long term.

You know what, I argued that we should use configs because this flag is incomplete. So I shouldn't be all up and down that it isn't.

I swear this is my last objection :) As soon as this is settled, I'll accept.

Now that I had ever more time to think about this patch, I see a great potential in it for development use, for example, we could silence a checker before splitting it up to see whether we could disable it altogether or really having to go through the process of splitting it into a modeling and reporting portion.

Thanks!!! Please note that BugReporter.cpp (especially the parts you touched) just got refactored extensively, so you'll need to rebase on master.

Hmm, why the need for checker options? Why not have them by default? If fixits are an experimental feature, maybe we should have a global enable-fixits config maybe. But I don't insist :)

In D66042#1629019, @alexfh wrote:

In D66042#1627760, @NoQ wrote:

In D66042#1627193, @alexfh wrote:

But without this patch clang seems to have the same two ANALYZE log lines regardless of whether I enable core checkers or not

Yup, it seems as if clang-tidy enables core as long as at least one Static Analyzer checker is enabled (even if it's path-insensitive).

It would be nice, if Static Analyzer would hide this from the users completely. The user would specify the list of checkers they need and CSA would enable whatever additional checkers it needs (if any) and then filter out their results. Is it feasible?

This really shouldn't matter much. Go for it!

If you have no major objections, could you please accept formally? :)

In D66042#1627842, @Charusso wrote:

Any analyzer config flag is equally accessible to anyone as the driver flags as they are both flags. The only difference is the config flags are more code to implement, and a lot more difficult to use. @NoQ, why the hell would we pick another type of flag which makes zero improvement? The goal is to introduce the best possible solution, which is already here.

LGTM, thanks! Do you need someone to commit this on your behalf? Also, could you please make the comments capitalized, terminated, and fitting in 80 columns?

In D61027#1627785, @gribozavr wrote:

Like @riccibruno said, check-clang-tools will run them. However, before committing a patch, please run check-all -- you never know what your patch can affect.

The deed is done.

We might also want to change the revision title, but the commit message for sure, to make it clear that this affects all checkers, something along the lines of "[analyzer] Add an analyzer config to silence diagnostics from user specified checkers/packages"

In D66042#1627765, @NoQ wrote:

In D66042#1626631, @Szelethus wrote:

In D66042#1626513, @Charusso wrote:

I really appreacite your ideas. It is unbelievable you guys bring up 20 different ideas for 5 LOC. I cannot really argue about any idea, as every of them could be a possible solution. I have to pick the right solution, and drop the other 19. I believe with that in mind what is an experimental feature and how we support to use the Analyzer, none of the 19 ideas would born. I did not want to refuse that many ideas, but I have to, because we could pick at most 1 to implement per patch. That is why I really try to emphasize it is under that experimental feature umbrella and we have to think no more about that patch from that point: since the beginning.

Given our discussion, we've thrown out all but 1 of the 4, by the way (fixing the actual problem, making this a config, creating checker/package options, solving this in scan-build only), ideas. Make this a config. You're correct, thats about 5 LOC change in this patch, at which point I'd be happy to accept :)

You mean something like -analyzer-config silence-checkers=core.DivideZero? I guess we can do that, right?

Oh, crap, forgot to address inlines. I'll see how buildbots react to these changes and will follow up in a smaller patch. Reopening as a reminder.

Apologies for going off-topic, but would it make sense to start lifetime related patches with the tag [LifetimeAnalysis] or similar (like [analyzer] for static analyzer patches)? I dont feel knowledgable enough to participate, but would gladly add a herald rule to be subscribed and learn about the development process of it.