And if we find out that this is far too painful, we can always put it back anyways. Doubt that'll happen though.

Hmmm, came across this one in the not too distant future, and always wondered how painful that performance hit would be that even Release+Asserts should be spared from it. I think 5% performance hit, if asserts are enabled, is an acceptable tradeoff, if the assert is crucial.

In D55400#1367046, @george.karpenkov wrote:

Hmmm, does this mess with options that bad? Could you please clarify?

registerChecker gets-or-creates a checker object. A checker name (used for getting the options) is set the first time it's created.
The checker which was created first "wins" and gets to name the resulting checker.
In practice it basically means that options and checkers reusing the same class do not work.
Do you have better ideas on how this could be arranged?

If you don't mind, I'd prefer to finally get over this patch. I'll commit on the 31st, but will wait for any potential feedback 'til then.

Poor wording on my end, sorry about that. Let me clarify.

Yup, I'm sold on that.

Cheers, this is as good as it gets! IRL we also mentioned making a unipage for each checker which would be awesome (and would require a lot of tedious work), but as a start, I'd much prefer browsing through this doc than the current one. Very much appreciated!

Rebase.

• Rebase
• Resolve the issue mentioned above by not enabling checkers that has any of their dependencies explicitly disabled
• Introduce osx.RetainCountBase to "solve" the issue mentioned in D55400#1364683, but the test files are still untouched. This leads to 9 lit test failures, but since this patch is ancient, I'd rather update it now and follow up with another, final version later.

Remove mutable specifiers from all fields of CheckerRegistry.

Rebase.

In D55424#1326397, @NoQ wrote:

Should we also pass CheckerManager into shouldRegister...? Or is it entirely useless?

I'm working on rebasing my dependency branches on top of trunk, and I'm somewhat stuck on this patch, could you lend a hand please?

Ping^3. @NoQ @george.karpenkov Could you please take a look? @xazax.hun Any other objections?

In D35068#811436, @NoQ wrote:

I wonder how noisy this check is - did you test it on large codebases? Because these functions are popular, and in many cases it'd be fine to use insecure functions, i wonder if it's worth it to have this check on by default. Like, if it's relatively quiet - it's fine, but if it'd constitute 90% of the analyzer's warnings on popular projects, that'd probably not be fine.

Overall I think this looks great, thanks! I left some inlines that would be nice to fix before commiting, but all of them are minor nits.

Ping^2

Awesome detective work! I glanced over the code, it looks great. I'd love to dedicate more time to your liveness-related patches, but university is a thing, so finding typos and the like is the best I can do for a while.

Always bothered me! Is there a way to squeeze this into %diff_plist?

Missed out on the developments. Thank you so much for handling this!

Thanks! Sorry about being a little slow with this, I'm sadly busier than I expected, but I'll definitely think about a nicer solution.

Sure! I'm on my phone, and will be for a little while, can you commit on my behalf?

I'm still looking for a sensible solution, but I'll at least share a patch that actually works.

Cheers!

Yea, should've used hasArg anyways. I'll look into this tonight.

This is awesome. Please wait for either @NoQ or @george.karpenkov to have the final say.

Global nit: I guess having both DESC and DOCS as variable/macro arg names is confusing. Can we instead use DOCSURI/DocsUri?

Thank you so much for working on this! The lack of a proper documentation is indeed a weak points of the project.

I still expect some skeletons to fall out of the closet

This patch doesn't handle -analyzer-disable-checker, which leads to assertation failures in later pathes. Since the way which checker should/shouldnt be enabled is implemented rather poorly, I'll probably try to find a long-term solution.

Ping.

Lit test failures are gone for Windows after reverting this. Will probably deal with this revision after 8.0.0.

Reverted in rC349344.

Reverted in rC349340. With the wrong revision name... oops...

A little late to the party, but I don't see anything against this.

@boga95 Do you have commit access, or need this to be commited on your behalf?

Interesting, I've been watching the bots closely, but got no mail after a while. I'm not sure what the cause is, so I'll revert one-by-one.

In D54834#1315475, @NoQ wrote:

Thanks, nice catch!

It seems that the ReportDoubleDelete() thing was never used for reporting double-delete, but it was used for some strange check when a destructor is called.

Hmm, I find the revision title and summary a little vague -- I'd expect a revision called "Refactoring" not to feature any funcitonal change, yet you changed how variadic functions are handled. Please

• Keep purely formatting changes to your earlier revision, and rebase this patch on that
• Edit the revision title and/or summary about what your patch does,
• If it features any change in the checker's behavior, include tests.

Commited in rC349274.

Hmm, actually, if you're doing changes all over the file in the followup patches, it shouldn't matter much. Let's just wait a couple days for ppl to object, now that they are subscribed.

That I think sounds great! Thanks!

I guess we used to have something that would result in an event duplication (which was the motivation for this patch), but it's long forgotten.

I don't really use scan-build, so I can't barrage you with suggestions, but the patch looks great.

Please don't mind me intruding.
If you create a patch for the Clang Static Analyzer, please add "[analyzer]" in the revision title, because many of us are automatically subscribed to those patches. Also, upload patches with full context.
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface