- User Since
- Jul 19 2017, 6:59 AM (131 w, 6 d)
Dec 23 2019
Dec 21 2019
I am not sure what would be the best way to test this change here.
Nov 21 2019
For reasons other than being a part of the project, CodeChecker is objectively an amazing tool to use with the analyzer. LGTM!
Nov 14 2019
Nov 12 2019
Accepted accidentally. Well, in any case, I trust your judgement on this! I'd prefer not to commit the test file as-is.
Nov 8 2019
LGTM, provided that the inlines are addressed! Thanks!
Nice catch! Though, wouldn't the memory sanitizer buildbots break on this reliably?
Nov 6 2019
Nov 5 2019
Hmm, so this checker is rather a collection of CERT rule checkers, right? Shouldn't the checker name contain the actual rule name (STR31-C)? User interfacewise, I would much prefer smaller, leaner checkers than a big one with a lot of options, which are barely supported for end-users. I would expect a cert package to contain subpackages like cert.str, and checker names cert.str.31StringSize, or similar. Also, shouldn't we move related checkers from security.insecureAPI to cert? Or just mention the rule name in the description, and continue not having a cert package?
Nov 4 2019
YES PLEASE. Debug checkers that only dump from check::EndAnalysis won't rely on the analysis not actually crashing. Which is ironically exactly when we want to use them.
Changes to MallocChecker really highlight the positive effects of this patch. Nice!
Oct 30 2019
Let's make it 3! Thank you so much for sticking by, I guess one of the reasons why a patch so obviously great and desired took so long is that we still didn't figure out how we imagine the CallDescriptionMap to be integrated into larger checkers :) In any case, this is a major step in the right direction, MallocChecker and some of the others should take notes.
I have no authority in clang-tidy, but the idea is nice! You may wanna reupload with full context though.
LGTM given that the inlines are fixed.
Its becoming a bit difficult to navigate inlines, could you please mark them as done as you address them?
LGTM, can we remove the open projects entry under the same breath?
Oct 22 2019
Oct 14 2019
Oct 11 2019
This is amazing, thanks!! LGTM.
I forgot to emphasise that the entire point of the patch was to get rid of getAllocationFamily, at least the way it used to work, because it was a mess and prevented me from moving forward with changing things to a CallDescriptionMap.
Oct 10 2019
Oct 7 2019
Oct 2 2019
Rebase, unforget to support 3-arg malloc, for which apparently were no tests at all.
Use regular parameters instead of template parameters.
Oct 1 2019
The logic of the patch is fine in my eyes, but I dislike the formatting. Could you please make the code obey the LLVM coding style, and after that use clang-format-diff.py?
A few nits inline, otherwise the patch is awesome, thank you!!
Sep 30 2019
I'm starting to be really happy with the current direction! I think I'll start splitting this up soon, I'm confident that the current interface (after some polishing) is general enough to develop incrementally.
Sep 28 2019
The patch looks alright, I won't formally accept because if I knew how these worked, it wouldn't have caused so much pain to so many people :)
Sep 27 2019
It seems like this patch is diffed against your latest commit, not the master branch.
Sep 24 2019
I have no objections on my end.
If you could change that, it would be awesome! But since this revision has its own side effect, let's commit as-is. LGTM!
Sep 23 2019
Hmm, so before this patch, we just used LVNode everywhere and ignored InputNode. It may not have made much sense, but its still not as confusing as using both if the creation of LVNode is unnecessary overall. Could we just remove it?
Never knew about this either, so I'm not gonna miss it. Thanks!
I really don't think so, but I'll ask around in the office.
The following words are echoing in my ears as I'm knowingly going completely against them:
Sep 21 2019
Apologies for the intrusion, but these plugins were added by me, and test an important case for our internal plugins. Could you please give me just a day or so the check whether the tests work for us correctly?
Sep 20 2019
Welp, windows builtbots broke again. I'll try to see whats wrong with undef sanitizer, but fear this will end up in a revert.
Sep 19 2019
In retrospect, I would have made this patch a little more fragmented (its almost a year old, does it beat the revival of the symbolreaper patch?), but it would be a painful chore to separate at this point, if you dont mind. I have a couple more cooking in the cauldron that are more digestible, so, lesson learned :)
Rebase, fix (suspected) error that caused buildbot errors. Hold off commiting in favor checking whether putting CallDescriptionMap in would be too invasive, but really, can't be worse then it already is.
Sep 18 2019
We have no solid evidence to conclude that such an event should not occur at a BlockEntrance, so fixing this error mustn't be that bad. I certainly should've used llvm::isa_or_nonnull, so the patch overall makes sense, so I'm commiting it as is. With that said, this test case highlights a fundamental flaw in how loop wideining is implemented (hence it being off-by-default), and it should be addressed later separately.
Sep 17 2019
Sep 16 2019
While we're there, could you please see whether the included test case (note how condition tracking is turned off) fails on your platform without the rest of the patch applied (it definitely does on mine, which is why I was surprised to see this bug report pop up only now)? If not, I can just push a small commit with the llvm::isa_and_nonnull change to get some breathing room.
Sep 15 2019
Sep 14 2019
Aha, so every user gets to create their own PathDiagnosticConsumerOptions object, makes sense! There is no interface misconception, because -analyzer-config will only configure what the analyzer would tinket with. I like this patch! If you dont mind, I'd prefer to have one last round with this, but otherwise LGTM.
Sep 12 2019
Yes, thank you! I've been keeping my mailbox open and commiting slowly, it seems like the buildbots have a wrong email address set up for me.
Sep 11 2019
@Szelethus: I could have stored PathDiagnosticConsumerOptions in AnalyzerOptions by value and pass a const reference around, but it wasn't pleasant to integrate with AnalyzerOptions.def. I.e., i'd have to implement a new kind of option that doesn't allocate its own field but instead re-uses a field within a sub-object. Do you want me to go for it or is this implementation good enough?
Looks great! Are we sure that PathDiagnostic.h is a good header name?
LGTM! I think code readability improved geatly.
I'm open to discuss a better design here. Eg., i thought about making it part of the visitor interface instead, but i don't immediately see how to do this without breaking the logic of "only add the note at the call site in which the event has happened, not every time allocated memory is returned from anywhere".