Aha, alright! So the tracker tracked back to where the tracked expression got computed to node N, which is the return value of some identity function. Unless its explicitly told that there is a link in between the return value and the parameter, the tracker can't figure this out that it could track further (why would it, right?). So it asks its handlers whether they want to do anything with N, which IdentityHandler does -- it digs out the node where the parameter was evaluated, and tells the tracker to go on. Awesome!

Sorry for the absence, took my time to catch up with this series! Really interesting insight into how a new interface is being designed! I need some time to digest things, but can't wait to help on the remaining patches.

I know I'm late to the party, and am just mostly thinking aloud. Awesome patch series!

Fix the test file.

This looks great!

While I still see checker silencing as a solution to a problem that shouldn't exist, I'm unsure about the amount of tedious and invasive work required to make the underlying infrastructure elegant just for the sake of it being elegant. Most notably, checker silencing solves the problem where a checker finds fatal errors -- disabling such a checker also gets rid of the sink node, which makes the entire analysis behave differently. This is desirable, as explained by @a.sidorin in https://lists.llvm.org/pipermail/cfe-dev/2019-August/063135.html. Although I still have concerns about the user interface if we promoted checker silencing to be user facing, it might be worth investigating a bit further.

Cheers!

Pushed this to the 12.0.0 release branch, just forgot to close it!

Neat!

As far as content goes, here are my thoughts:

LGTM! You may wanna wait for someone more knowledgeable about CTU to give it another accept.

ping

I don't insist on this patch, though I will end up removing the FIXME even if I leave the actual code unchanged, as it seems to be outdated.

Looks like a neat checker! I guess the only question is the function matching, and I don't dislike it in its current state. @martong, do you have any thoughts on this?

LGTM! As a side note, are we aware of anyone that uses short messages instead of the full length one?

Happy to see this mess go. It was obvious even after the first few hurdles that it wouldn't work out in the long term.

My no1. thought is that I wish the new functionality you're implementing was in the interface of the Preprocessor. I found, and still find it so hard to believe that you couldn't just retrieve this information -- your projects seems to plug this gaping hole in the design. I respect that this is an opt-in functionality for now though, and I guess we could make this a default feature with relative ease.

This is amazing. We longed for a sensible implementation for this for a long time. Really liking the unit tests as well!

Fix'd!

I don't have the time to comb through this doc, unfortunately, but I want to applaud this effort to make the iterator checker family more accessible. Its certainly a forerunner in modeling tricky C++ constructs, and I can't wait to be a more valuable reviewer after being a bit more knowledgeable about it.

In D89987#2350649, @NoQ wrote:

Honestly i'd rather eliminate SymExpr and go with Symbol everywhere. It's an overloaded term but appending "Expr" to it doesn't really make it significantly less overloaded.

I'll just add a generic comment because I don't know much (==anything) about webkit checkers. Do you expect to stumble across false positives regularly enough so that an attribute like this would be necessary? Do these checkers rely on some heuristic?

Sounds good! Thanks!

In D72705#2274568, @balazske wrote:

I am not sure if this checker is worth to further development. A part of the found bugs can be detected with other checkers too, specially if the preconditions of many standard functions are checked (more than what is done now) and by modeling the function calls with assumed error return value.

@martong has been working hard on libc support for the analyzer, I'll add him :)

Congrats on your GSoC! Unless I missed it, it seems like you haven't posted your final report on cfe-dev, even though its an amazing looking document with a lot of examples and explanations. I think it would be great to spread the message, its a work to be proud of!

Yup, we've talked about this before. This is indeed a better interface design. LGTM!

A joy of reviewing C++ code is that you get to marvel in all the great things the language has, without having to pull fistfuls of hair out to get get to that point. These patches are always a treat. LGTM!

LGTM! Thanks!

The fix is great, thank you so much! The test seems to be annoying, it might be worth looking into it some other time, but that is definitely orthogonal to this patch.

I figured you're still working on this, sorry! I'd really like to chat about my earlier comment D71524#1917251, as it kind of challenges the high level idea.

There are some immediate observation to the patch, like a new kind of check definitely warrants a different warning message. However, I see that this is WIP patch, so I'll try to keep this high level.

Post-commit LGTM on the post-commit changes!

Sorry for the slack (which is kind of ironic with my attention grabbing title :) ). Landed in rG791b7e9f73e0064153a7c3db8045a7333a8c390c. Thanks everyone!

@balazske may have some closing words.

Rename the live statements checker to live expressions checker. The test file added in a revert commit changed rather heavily, but it makes sense that these entries are removed IMO. Unless somebody objects, I intend to land this as-is.

Reinstate the assert removed in rG032b78a0762bee129f33e4255ada6d374aa70c71, add a test. Enforce that Environment only makes an exception for ReturnStmt in terms of non-expression statements (and hope that isn't going to stay that way for long).

In D86736#2257880, @NoQ wrote:

i'm very glad that we now consistently use expressions in our Environment.

In D79431#2263693, @martong wrote:

In D79431#2263690, @martong wrote:

What if we'd add a toString method to the constraints and we'd add this to Msg? This way we'd know the contents of the constraint, thus we we'd know how the constraint is violated.

I mean we'd know what is not satisfied. But, to know why exactly that is not satisfied we should dump the whole State but that's obviously not an option. Perhaps we could track which symbols and expressions are participating in the assumption related to the constraint and we could dump only those, but this seems to be a very complex approach.

LGTM on my end!

In D86135#2259325, @steakhal wrote:

Perfectly clear, thank you. However, I would still rely on the others to accept this :|

BTW why does the plist-macros-with-expansion.cpp.plist change? It makes the diff somewhat noisy :s

This is exactly how I imagines weak dependencies to work. LGTM on my end.

Added some documentation to the code snippet pointed out by @martong.

Added a line about D78933.

I can only imagine how long it took for you to write all this, because reading it wasn't that short either. Thank you so much! It really gave me a perspective on how you see this problem, as well as what is actually happening (and should happen) in the checker.

@NoQ in particular has been working hard on the common infrastructure in between the static analyzer and clang-tidy, I'll add him :)

I'm in favor of most, if not all of the changes, though I will admit that this patch seems pretty cluttered, you are doing a lot of refactoring under the same hood. You're moving, adding, removing and changing helper functions and their invocations. Would be possible to make this patch a bit leaner?

@balazske seems to be very involved, he might have some closing words -- from my end, LGTM!

The tests look great, thanks! I still lack the confidence to accept, unfortunately.

The patch looks great, in fact, it demonstrates how well thought out your summary crafting machinery is.

LGTM! Nice!

In D72705#2241542, @balazske wrote:

The summary of this last discussion is that it is not acceptable to have only the simple check for the explicit comparison with a fixed constant. At least not for return types where the "implicit" check (a check that is always true or false for the error return value) is possible, for example the char* case.

In D86736#2249920, @martong wrote:

I don't have anymore immediate concerns, but I will need more time to comb through the rest of the patch in more details... hopefully I can do that in the following days.

Before we dive into this too much, if you can point to discussion that explains why we have a 2 versions of the same checker, that would be nice. Why did you chose to work on this one, and not the other? I recall us talking about this in a meeting, but its always great to have it set in stone.