- User Since
- Sep 17 2012, 3:16 AM (366 w, 1 h)
Tue, Sep 3
Tue, Aug 27
Mon, Aug 26
It looks like this solution is not going to work in general. The problem is that it can be really hard to tell when it is valid to create a gsl::Pointer from an unannotated local type.
Aug 23 2019
Committed in https://reviews.llvm.org/rG6379e5c8a441 due to it was urgent for some users. Will address any comments post-commit.
- Added a test.
- Add the actual diff.
Sounds good! Let's do that :)
Yeah, the analysis would work fine in this case. But that would mean that we should not propagate the Pointer annotations to derived classes as some of them in the wild do not follow the same ownership model.
Aug 22 2019
Aug 21 2019
Aug 20 2019
LGTM! I think the UIs could do better displaying this info in the future but this is not your job :)
Ok, I think I know what is going on.
Aug 19 2019
I do not know if there are some build bots with -Werror that would warn for unknown attributes. In case it fails the build bots we could extend the include/llvm/Support/Compiler.h header with a macro.
Aug 15 2019
Aug 14 2019
Aug 13 2019
LG! But let's wait for Dmitri :)
The changes look good to me. Added some folks who might have more knowledge about the original purpose.
Aug 12 2019
Aug 11 2019
Aug 9 2019
Aug 8 2019
- Fix review comments.
As this thing will now be part of the checker interface it would be great to have some guidelines which interestingness kind to use (or, when to use interestingness at all). I am totally fine with addressing this in a followup patch though.
Yeah, this is important bike-shedding from the user point of view. Otherwise it looks good to me.
Aug 7 2019
Aug 6 2019
Jul 29 2019
More tests are always welcome :)
I like the change.
Just wondering, did you check if we actually need shared ownership for this type? If not, do not waste your time checking for now :)
Looks good, some nits inline. I agree with Artem, we should consider omitting the trimming and document how to get something similar if desired for debugging.
LGTM! I also do not see much value in the old code at this point. I would expect PathDiagnosticConsumers to be independent of the interesting symbols/regions.
The direction looks good to me. I wonder if it is worth to add some warnings somewhere that it is really tricky to come up with reasonable fixits for most path sensitive checks.
Thanks for the review!
Since I did some refactoring I will wait for an additional accept before committing.
- Rebase, add the results of testing on real world projects to the description.
- Add new line to end of file.
- Rebase, calls no longer need special handling.
- Actually move the code snippet in question.
- A small refactoring based on an observation from a review comment.
Jul 24 2019
- Remove unused parameter.
- Fix a false positive case found by running over ~200 open source projects
I have run this successfully on ~192 open source projects.
Jul 22 2019
- Fix a false positive from previous change.
- Fix a TODO and add some more tests.
- Address the rest of the review comments.
Jul 17 2019
Since we allow new kinds of SVals to be tracked it would be great to test this first on a larger corpus of projects just to see if there is a crash (due to an unhandled SVal type).
I have one small question otherwise looks good.