- User Since
- Jul 19 2017, 6:59 AM (204 w, 5 d)
Thu, Jun 17
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!
Wed, Jun 16
Mon, Jun 14
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!
Fri, Jun 11
May 21 2021
Fix the test file.
May 20 2021
This looks great!
May 19 2021
May 18 2021
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.
May 13 2021
Pushed this to the 12.0.0 release branch, just forgot to close it!
Mar 4 2021
Feb 18 2021
As far as content goes, here are my thoughts:
Feb 16 2021
LGTM! You may wanna wait for someone more knowledgeable about CTU to give it another accept.
Feb 15 2021
Feb 12 2021
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.
Feb 10 2021
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?
Feb 9 2021
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!
Feb 5 2021
Nov 23 2020
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.
Nov 2 2020
Oct 27 2020
Oct 19 2020
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?
Oct 14 2020
Sounds good! Thanks!
Oct 12 2020
Sep 29 2020
Sep 28 2020
@martong has been working hard on libc support for the analyzer, I'll add him :)
Sep 25 2020
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!
Sep 22 2020
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!
Sep 21 2020
Sep 20 2020
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.
Sep 16 2020
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.
Sep 15 2020
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!
Sep 14 2020
@balazske may have some closing words.
Sep 11 2020
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).
Sep 10 2020
Sep 9 2020
LGTM on my end!
Sep 7 2020
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.
Sep 2 2020
Sep 1 2020
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.