- User Since
- Jul 19 2017, 6:59 AM (159 w, 4 d)
Fri, Aug 7
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.
Thu, Aug 6
Yeah, this looks straightforward, but how come you didn't manage to get a small test case? creduce gave up?
Tue, Aug 4
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!
Tue, Jul 28
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.
Lets make sure that we invite others from the community to participate, here and on other patches as well. Otherwise, LGTM.
Fri, Jul 24
Wow, I never realized I accidentally landed that assert (D82122#2172360), but I guess its great to have that covered. Would you prefer to have that reverted as I'm looking to fix this for good?
This was accidentally committed as a part of rGb6cbe6cb0399d4671e5384dcc326af56bc6bd122.
Thank you so much, you really went out of your way to dig that out! I'll try to patch this somehow.
I personally preferred the previous diff, the reporting part of the checker and the string length modeling was separated far more cleanly.
Thu, Jul 23
You could do it in the code, but if the modeling wouldn't be present from CStringModeling, CStringChecker wouldn't work properly. So you should make it a strong dependency, just as you did in this patch. My comment was mainly a response to @NoQ :)
Yay! This checker has become a major headache as the analyzer grew. Not on a RetainCount scale, but on a MallocChecker one. Cheap shots, I know :)
Wed, Jul 22
Ouch. Let me know how severe this is, because this is a big milestone in my project.
Tue, Jul 21
Mon, Jul 20
I chased my own tail for weeks before realizing that there is indeed another instance when a live statement is stored, other then ObjCForCollectionStmt...
I find the summary here a bit lacking. We detected this issue in D83120, where a lot of discussion can be found, so its worth linking in. On its own, I'm not immediately sold on whether this is the correct solution, and even if it is, how does it solve the problem. I bet you had to struggle a bit to understand the related machinery to fix this, it'd be invaluable to share the knowledge you gained here as well.
Sat, Jul 18
I hope you don't mind if I bring up a point you mentioned during a daily meeting.
Thu, Jul 16
I suppose LLVM could be a HUGE project?
I don't speak snek, but I approve this message!
Wed, Jul 15
Please do not bypass the previous comments that hadn't reached a conclusion -- littering inlines about miscellaneous stuff at this stage does more harm then good, and derails the discussion.
While this patch spans across a lot of files, it is actually rather straightforward. I'm stunned to see that we never really had a proper dynamic size support, especially that this patch has been out there for a good long time :) Oh well, better learn that now than never.
Unless @NoQ has anything else to add :)
Now that we found the answer to the only lingering question this revision raised, I think you can safely land it while we start looking into fixing this newfound bug. LGTM.
Tue, Jul 14
@Eugene.Zelenko Thanks for cleaning up revisions -- same as D69560#1725399, we are working in the same office and have worked on some forms of static analysis for a while now. Adding us as reviewers helps this revision being more visible. Its clear that we don't have the authority to approve changes to clang-tidy, but we can confidently request changes or add to the discussion while still respecting that.
Lets just postpone moving out-of-alpha to a standalone revision. That, without any other changes demands discussion.
LGTM! Very well done!
Aren't StackHints basically ancient NoteTags?
I've seen you resurrecting the thread, I'll just take a bit of time to remember what happened in the previous episodes!
Mon, Jul 13
I lack the confidence to accept this straight away, but the overall direction looks great. LGTM!
Please know that the criteria for moving checkers (or anything, really) out of alpha includes testing on open source code bases. Could you create a public CodeChecker link with some evaluations? How does this, if at all, interact with other alpha array bound checkers?
There is a parallel discussion in this patch on how we should cater to user requests, I wrote a lengthy comment about my opinion, but I just don't think it adds much -- at the end of the day, it is fair that if we implement an feature for a small subset of users that is known to cause unpleasant side effects, we shouldn't make it a part of the default user interface, because among many other things it inevitably forces other contributors to maintain it, even if they weren't enthusiastic about the idea, but its also fair to prioritize direct users within reason.
Jul 10 2020
I'm yet to go over line-by-line, but the overall logic looks great.
Its a bit hard to judge this. Have you tested this on open source projects?
I don't have much to say here, this goes a bit outside my expertise. @NoQ?
I'd prefer if you moved f_leak_2 to stream-notes.c. Otherwise, LGTM.
LGTM! Though, the test file change is interesting. You could add a test that behaves differently from the previous implementation:
Jul 9 2020
LGTM, but if we knowingly a subpar solution, we should make that clear in the surrounding code. I know the followup patch is around the corner, but just to be sure.
I don't like this solution toooo much, but I respect the urgency, and since we don't introduce a compatibility issue, I trust we can remove this if we decide to change to TableGen. Post-commit LGTM!
After reading @martong's comment D72705#2035844 and @balazske's comment D72705#2086994, I think there is a need to argue across all paths of execution within the function, and as such this is a dataflow problem.
Jul 6 2020
I'm on it!
LGTM! You packed a lot of punch into this patch, and I like it very much. I read the code and everything looks great. I nitpicked on one thing, the updateTrackedRegion function is a bit awkward, but if you place a TODO there, I'm happy to have that addressed in a later patch. I don't think this is reason to postpone the landing of this any further.
Jul 4 2020
Jul 3 2020
Small fixes according to reviewer comments.
Jul 2 2020
Mind that Signature in StdLibraryFunctionsChecker aims to achieve precise function call matching as well, and at the moment its a bit ahead. Some discussions can be found in D77410, D81745 and https://bugs.llvm.org/show_bug.cgi?id=46253.
I only looked at the checker naming issue, and that seems to have been resolved perfectly, thanks! :)
Jul 1 2020
The changes proposed in mailing list thread (http://lists.llvm.org/pipermail/cfe-dev/2020-June/066017.html) seem ortogonal to this change. Sorry for the slack today, I just peeked around in MallocChecker to look for conflicts, but it seems like there isn't any :) LGTM!
Jun 30 2020
Revert changes to plugins and to the related tests, allow the example package to bypass the new restriction.
Fixes according to reviewer comments!
Jun 29 2020
Note to self: rename debug.DumpLiveStmts to debug.DumpLiveExprs. Thanks for the review btw! I don't immediately have something to add to your discussion though :)
I just checked BuiltinBug -- why do we even have this??? Anyways, LGTM. We should probably delete the whole thing.
Jun 28 2020
I have a few pending patches that enforce some long desired restrictions on which checkers can emit diagnostics, and I'd prefer not to mess with your checker after you land this :/
Jun 25 2020
This revision has done its job.