- User Since
- Sep 17 2012, 3:16 AM (422 w, 6 d)
Sep 11 2020
LGTM! Looks a lot cleaner this way.
So now all of the dependencies are landed and this should be ready for its prime time, right?
Aug 31 2020
Aug 24 2020
LGTM with a nit.
Is this related to https://bugs.llvm.org/show_bug.cgi?id=44493?
Aug 22 2020
Please add a test case, where the unique_ptr is initialized from a pointer parameter that has no assumptions. I think that case is not handled correctly.
This is what I had in mind, thanks!
Aug 21 2020
Aug 20 2020
MemRegion is a popular class to instantiate in the analyzer so it looks good to me.
But unless you add a comment one will probably replace the offset with an optional as the part of a refactoring.
Aug 19 2020
Exactly, but you could return a StringRef to static storage.
I wonder whether having a virtual method for symbols to get the prefix would be cleaner (something like getKindCStr), but I do not insist.
Aug 17 2020
Thanks! Sorry for not noticing this earlier. The fix looks good to me and indeed the original intention was to report dead functions with ORE.
Aug 11 2020
Aug 10 2020
Aug 9 2020
Looks reasonable to me, but I am not very familiar with the impacts of the additional casts. Do we lose some modeling power when we are using the regular constraint solver?
Aug 7 2020
I think all the problems that left should be solved in a separate patch or even out of the scope for the GSoC.
Aug 4 2020
Jul 28 2020
I think adding code snippets that are affected by this patch would make it much easier to evaluate the changes and whether this is a good idea or not.
Jul 23 2020
Jul 21 2020
Jul 20 2020
Jul 15 2020
Jul 14 2020
Jul 7 2020
Jun 30 2020
Yay! This looks good to me and I love statistics, so a huge +1.
I have one question though. Isn't this counting all the reports in an equivalence class? I.e. if the analyzer finds something multiple times it will only be displayed once but here it will be counted multiple times. I think it might be worth to have two statistics, one for all bug reports and one for equivalence classes. What do you think?
Jun 29 2020
The patch looks good to me and I find it a welcome change that should make things easier to understand and maybe even a bit more efficient, I hope this won't break ObjC :) My discussion with Artem is orthogonal to this change.
Jun 26 2020
Jun 25 2020
I always wondered if we actually need to track the liveness of exprs at all. We could just kill all subexpr at the end of the full expression without precomputing anything. But I might miss something.
The analyzer inlines small functions within a TU regardless of the thresholds. I think it would be sensible to do the same across TUs in the case we don't do this already.
Jun 24 2020
I only checked the test cases and the comments so far and it looks very useful and promising. I really hope that the performance will be ok :) I'll look at the actual code changes later.
Jun 14 2020
Jun 12 2020
I would not call the results of the measurement within the margin of error but the results do not look bad. Unless there is some objection from someone else I am ok with committing this but please change the title of revision before committing. When one says optimization we often think about performance. It should say something like reasoning about more comparison operations.
Jun 10 2020
Jun 6 2020
Please add the [analyzer] tag in front of your patches as some folks have automated scripts based on that tag to add themselves as subscriber/reviewer.
Jun 2 2020
May 29 2020
A high level comment.
May 26 2020
May 19 2020
Thanks for finding this bug! Adding some reviewers.
May 13 2020
May 8 2020
I still have some nits inline, but overall the implementation looks good to me.
I think, however, that we should not only focus on the implementation but on the high-level questions.
Is this the way forward we want? Can we do it more efficiently? What is the performance penalty of this solution?
Overall looks good to me some questions and nits inline.
May 7 2020
May 6 2020
I do agree that the warning message is not the best but it is not horrible either :)
Thanks for working on this, I do believe the analyzer would greatly profit from better constraint solving capabilities. Unfortunately, we had some troubles in the past trying to improve upon the current status and we had to revert multiple patches. This is why the community is super cautious when it comes to changes like this.
I am a bit unsure about this change. C libraries might be consumed in C++ projects and C++ projects might be free to overload those functions. Does the effort needed to specify the signatures justify not supporting that (probably unintentional) name collisions in C++?
This is cool!
I'd prefer to have this functionality committed together its first actual use with tests.
Also, I see no tests :)
If a given parameter in a FunctionDecl has a nonull attribute then the NonNull constraint in StdCLibraryFunctionsChecker has the same effect as NonNullParamChecker.
I am a bit unsure what the purpose of these Max summaries are? As far as I understand the Max represents the largest value for the type of the formal parameter.
I think testing summaries this way can be really hard to manage in the future.
I see two possible ways forward to make this easier:
a) Make something like https://reviews.llvm.org/D78118 that will also dump the actual summary in a textual form, not only the fact that a summary was loaded and check for that textual form.
b) Make a small helper library for testing summaries. E.g., most of the buffer handling functions have similar summaries, so we could have only a couple of functions for testing buffer related summaries and we could just pass in function pointers (as templates or params) instead of duplicating code. Similarly, testing output ranges could be generalized.
Overall looks good to me, I have one question inline.
May 4 2020
Apr 30 2020
Nice catch, LGTM!
Apr 29 2020
Overall the changes look good to me here. I had a small nit inline. But I wonder if we actually should add more code in the analyzer core to better model the sizes of memory regions corresponding to the VLAs.
Apr 24 2020
Apr 23 2020
Overall looks good for me, thanks for tackling this problem! I think this should be good to go once Eli's comment is fixed.
I think this should not be blocked on the gtest update. If getting an updated gtest into the repo takes to much time and the reviewers are happy, I'm fine with doing that change as a follow-up.
Actually, sorry. I just realized that the alignof problem is introduced by this patch. I'd love to see the solution committed together with this patch (it could be a separate patch but preferably, they should be committed together.)
Thanks! Having more tests is always welcome!
Thanks, this looks much better now.
Could you also update the description of the revision to match the current status? (E.g. type aliases are now supported.)
If you do not plan to solve the alignof issue in the near future, maybe it would be worth to also open a bug report.
Please wait for one more approval before commiting.
Apr 21 2020
Ok, looks good to me.