In D64680#1590619, @NoQ wrote:

Great, thanks!

• More fix.

• Fix.

In D64680#1584315, @NoQ wrote:

P.S. I think you should attach the report to Phabricator directly, as the link will expire as soon as these reports get regenerated.

• Remove unnecessary DoNothing kind.

Here is an example of the mentioned use-after-free by pointer-escaping as an argument:
https://llvm.org/reports/scan-build/report-DeclBase.cpp-getFromVoidPointer-0-1.html#EndPath

In D64680#1584076, @NoQ wrote:

Change of plans: let's suppress the warning when our free() is done within the function that has __isl_take in its definition. So, like, ascend the chain of location contexts and check your callers when you're about to mark the pointer as released. If any of the callers contain __isl_take, mark it as escaped instead.

• Fix.
• Move the logic to free() for better matching.

The impure-warning sounds like some alpha, not-well-defined warning, other than that I like the movement to rethink existing checkers.

Much better! Thanks!

• Add the llvm namespace to the test file.

Thanks for the review!

In D64374#1577266, @NoQ wrote:

Can you provide more info, eg. the full backtrace?

• Add the forgotten llvm namespace to the CallDescription.

In D64374#1577235, @NoQ wrote:

Whoops. I underestimated you (:

Ok, anyway, this was my last comment. Great job! Remember to make sure that it works on the actual LLVM, not only on tests; mocked-up test headers are very easy to get wrong.

• Move to apiModeling.llvm.
• Prevent unknown casts.
• Refactor.

• Simplify the new getNoteTag().

• Fix.
• More tests.

• Fix a typo.

Thanks! My mind was really set to actually model these with classof(), whoops.

• Fix
• New getNoteTag() which accepts a plain note.

Document!!4!44! It is great you have started to limit the notes, thanks!

May you would remove the "Program points:" part as we only have that, but I think we have enough space for that tiny note. Thanks!

Smart move! The left-most usually the shortest path, but what if not? I would calculate the length, other than that, cool. Thanks!

Thanks for the reviews!

In D63915#1569410, @Szelethus wrote:

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

Yes, it is made for LLVM and tested out 4 times. Thanks!

• Done.

Well, it is always awesome to think about the others. Thanks!

Thanks for the reviews! The remaining question is: do we want to use Optional<> in the CallDescriptionMap::lookup()?

• More fix.

• Fix.
• Document NoteTag.

In D63915#1568166, @Szelethus wrote:

This checker seems to only check LLVM functions, but doesn't check whether these methods lie in the LLVM namespace. Is this intended?

Thanks for the reviews! They are not in the llvm namespace.

• Fix.
• Refactor.

In D64100#1567706, @NoQ wrote:

In D64100#1567230, @Charusso wrote:

Nice! Could you add some #===---===# separators, please? As it is in the finishing state, I think now it is appropriate.

rC364991.

• Create FunctionExitPoint diagnostics.
• Fix.

In D64110#1567505, @NoQ wrote:

A non-sink node may have no successors because it ran out of statements to execute (i.e., the analysis finishes successfully).

Ah, of course, sorry.

I have removed that is_sink business because we have an actual graph, not just a huge painting, so you could mark whether it is sink by it has zero successors. I like the idea about refactoring that crazy code, thanks!

In D64104#1567326, @NoQ wrote:

In D64104#1567317, @Charusso wrote:

I like it! What about BlockEdge with the long terminators? (c.f. Edge.getSrc()->printTerminatorJson())

We don't process these yet >.< But it should be trivial to cover them in a similar manner once we implement them.

I like it! What about BlockEdge with the long terminators? (c.f. Edge.getSrc()->printTerminatorJson())

Nice! Could you add some #===---===# separators, please? As it is in the finishing state, I think now it is appropriate.

I really like that, thanks you! When you spend tons of time it is good for your eyes and that dark theme is very cool in 2019.

• I do not like Optional<const T *> anymore.
• More simple notes.

In D63915#1563049, @NoQ wrote:

Aha, nice, thanks for adding a description, it is a very good thing to do. Like, every commit should be obvious.

In some of my patches I have not added a description because they are so tiny changes.

• Fix.

Thanks for testing it!

Are you alright with the namings? Those are randomly came out from my head and may you know some better one.

SSS!

Nice, the end of that huge work is getting closer. Thanks!

• Revert the Calls option. It turned out too difficult to create. (May it useless, as we have tons of similar checkers without any option.)

I have not seen any problem. Thanks!

You have made a great use of that mistake by me. So that is why everything is so perfect, because it fires, just it should fire later. Thanks you!

Great patch, thanks you! I wanted to make my own IdentifierInfo array previously.

Good idea! Thanks!

• Fix the note as the class name is optional.

• Make it match the class name. (Whoops!)
• Reorder the tuple (swap the return value with the call name).
• Remove the Projects option.
• Remove unnecessary input validation.

Heavily WIP, see inline.

• A working one.

In D63462#1556831, @NoQ wrote:

See also https://xkcd.com/1638/

• More test.

In D63727#1556821, @NoQ wrote:

I think i'd rather remove it entirely.

In D63726#1556825, @NoQ wrote:

Yay thx!

For Environment they aren't that useful, but it definitely doesn't hurt.