User Details
- User Since
- Jul 19 2017, 6:59 AM (182 w, 5 d)
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
LGTM! Thanks!
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
LGTM! Nice!
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.
I'd prefer to have a couple more green lights. In particular, another look on the constraint manager and Objective C stuff would be great.
Aug 31 2020
I'm not thrilled by this solution. As I understand it, the assertion was put there to enforce our ability to create a new assumption, and its great that we had it, because we managed to catch a fault in that. Seems like this patch is treating the symptoms instead of curing the illness. Given that the code would probably work fine in builds with assertions disabled, I'd prefer to take a look at where the actual problem is. If it turns out to be unsolvable or require unreasonable amount of work, we can still decide to land this as-is.
Aug 27 2020
Fixes according to reviewer comments!
Nice, thank you! Did you stumble across this, or found it with a tool?
Aug 26 2020
Fixes according to reviewer comments!
I debated this report with @bruntib, depending from where you look at it, its either a false positive or not -- in short, this is what happens:
char *p = /* some assumed to be valid c string */ end = strchr(p, '\n'); if (end - p > 4) { // <-- warning here on end's invalid use }
the guarded code wouldn't be executed, should end be zero. Now, if we interpret the rule in a very literal sense, that
Aug 25 2020
I'll get the nits I didn't object to fixed, thats the status you're looking for :)
@vsavchenko I just realized a significant portion of your work for this release is the following list:
git log llvmorg-11-init..llvmorg-11.0.0-rc2 --oneline -- clang/utils/analyzer/
Is it a correct guess that while your primary audience are the analyzer developers, it wouldn't hurt to mention it in the release notes?
We will need to push this to the 11.0.0. release branch as well, sorry for the trouble.
Thanks! I'll get these fixed.
Documentation under clang/docs/analyzer/checkers.rst seems to be missing. Could we get that fixed before 11.0.0 releases?
Aug 24 2020
What a legacy.
Do I sense correctly that the only information CSrtingLengthModeling.cpp requires from the actual CStringChecker is a checker tag? Because if so, I think we should just separate them even more cleanly -- we could just make a CStringLengthModeling checker implement the checker callbacks in that cpp file and we wouldn't need to move CStringChecker to a header. Not that moving it there is fundamentally bad, but in this instance it seems like we're legalizing bloating checkers instead of separating them. Also, this patch seems to have a significant overlap with D84979 -- is this intentional?
I really like the patch, but have nothing to add to what other reviewers already said. Nice!
Aug 18 2020
Aug 14 2020
Unless anyone else objects, I'm happy to see this landed! Nice work! ^-^
Aug 13 2020
Ah, okay, silly me. Didn't catch that. Then the message is OK for now.
Shouldn't we create a new test care for this, instead of expanding an existing one? Btw, this looks great, but I lack the confidence to accept.
Lets make sure we invite the wider community to see whats going on. Otherwise, LGTM!
Tests c:
The patch looks great. I guess the only remaining discussion remains as to whether this should be in libAnalysis, or somewhere else. Here is my take: Since clang-tidy, CSA, and some other parts of the compiler (like uninitialized variable warnings) are static analyzers within the same infrastructure, it makes sense for them to have a common library. I see that diagnostics aren't really analyses. libFrontend or libDriver, which, as I understand it contains most the diagnostics machinery for clang aren't viable alternatives because of the library dependencies. So, I think if libAnalysis was called libStaticAnalysisCommon, we would be golden, but that would screw over downstream developers for negligible gain. While I'm not terribly experiences in library layout design, for the time being, the move to libAnalysis seems appropriate.
Aug 7 2020
Layering violations are a running theme in the analyzer -- CheckerRegistry and the entire MallocChecker fiasco are two glaring examples. Luckily, this isn't a severe case so I wouldn't worry about it much.
Aug 6 2020
Yeah, this looks straightforward, but how come you didn't manage to get a small test case? creduce gave up?
Aug 4 2020
LGTM on my end, but please wait for approval from either @gamesh411 or @NoQ.
Would it be possible to publish these results on a public CodeChecker server?
We definitely need more tests. The idea overall however is amazing, thanks!
Jul 28 2020
Let me double down -- just because we let some select people know about an option, I still don't think it should be user facing. I'm strongly against the philosophy that a core-modifying decision, such as configuring a state split should be presented on the default interface. Having "smart users" isn't an excuse to that either -- the last thing I'd like to see broadcasted is that you need to be a genius* to use or configure a tool that is suppose to serve you, not the other way around.