[PATCH] D150446: [analyzer] Check ArraySubscriptExprs in ArrayBoundCheckerV2

donat.nagy created this revision. donat.nagy added reviewers: dkrupp, gamesh411, steakhal. Herald added subscribers: manas, ASDenysPetrov, martong, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun. Herald added a reviewer: NoQ. Herald added a project: All. don

[PATCH] D150446: [analyzer] Check ArraySubscriptExprs in ArrayBoundCheckerV2

donat.nagy added a comment. After some thinking and internal discussion I decided that it'd be better to replace the callback used by this checker. This is a clean but rough draft of this concept; for a final version I'd consider: - adding a secondary callback that handles `*ptr` equivalently

[PATCH] D150446: [analyzer] Check ArraySubscriptExprs in ArrayBoundCheckerV2

donat.nagy updated this revision to Diff 521640. donat.nagy added a comment. //(Re-uploaded patch to apply clang-format changes and remove a stray temporary comment.)// Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150446/new/ https://reviews.llvm

[PATCH] D150446: [analyzer] Check ArraySubscriptExprs in ArrayBoundCheckerV2

steakhal added a comment. I still struggle to see the motivation. Please consider adding a test demonstraing a case when the diagnostic is not relevant or easily misunderstood by the users. Alternatively, give an evaluation of this change so that I can see it myself. In the end it would still m

[PATCH] D150446: [analyzer] Check ArraySubscriptExprs in ArrayBoundCheckerV2

donat.nagy planned changes to this revision. donat.nagy added a comment. I started to create tests to demonstrate the differences from the baseline, and it turns out that my patch is handling multidimensional arrays incorrectly. I'll debug this issue and upload a fixed version of this patch. R

[PATCH] D150446: [analyzer] Check ArraySubscriptExprs in ArrayBoundCheckerV2

donat.nagy updated this revision to Diff 522544. donat.nagy edited the summary of this revision. donat.nagy added a comment. I managed to debug the issue that plagued my commit (see `test_multidim_zero()` for details, I think I'll fix it in a followup change); and I have uploaded a new version o

[PATCH] D150446: [analyzer] Check ArraySubscriptExprs in ArrayBoundCheckerV2

donat.nagy added a comment. By the way, I'm fed up with the hack that ElementRegion is used for three separate things ("real" array indexing, casts and pointer arithmetic) and I'm thinking about introducing a subclass hierarchy where a base class `ElementLikeRegion` has three subclasses called

[PATCH] D150446: [analyzer] Check ArraySubscriptExprs in ArrayBoundCheckerV2

steakhal added a comment. In D150446#4345723 , @donat.nagy wrote: > By the way, I'm fed up with the hack that ElementRegion is used for three > separate things ("real" array indexing, casts and pointer arithmetic). To fix > this I'm thinking about intr

[PATCH] D150446: [analyzer] Check ArraySubscriptExprs in ArrayBoundCheckerV2

donat.nagy added a comment. I'll move this class hierarchy modification question into a discourse thread after collecting the relevant facts. (By the way, thanks for suggesting discourse, I was not aware of it!) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews

[PATCH] D150446: [analyzer] Check ArraySubscriptExprs in ArrayBoundCheckerV2

donat.nagy added a comment. @steakhal What is your first impression about this commit? Is this the (or at least //a//) good direction? May I continue with creating additional improvements (e.g. better error messages) that depend on this commit? Repository: rG LLVM Github Monorepo CHANGES SI

[PATCH] D150446: [analyzer] Check ArraySubscriptExprs in ArrayBoundCheckerV2

donat.nagy abandoned this revision. donat.nagy added a comment. Herald added a subscriber: wangpc. I'm abandoning this commit because it amalgamates several unrelated changes and I think it'd be better to handle them separately: 1. First, there is a very simple, independent improvement related t

[PATCH] D150446: [analyzer] Check ArraySubscriptExprs in ArrayBoundCheckerV2

steakhal added a comment. In D150446#4560892 , @donat.nagy wrote: > I'm abandoning this commit because it amalgamates several unrelated changes > and I think it'd be better to handle them separately: > > 1. First, there is a very simple, independent imp

[PATCH] D150446: [analyzer] Check ArraySubscriptExprs in ArrayBoundCheckerV2

steakhal added a comment. I'll come back one or two weeks later. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150446/new/ https://reviews.llvm.org/D150446 ___ cfe-commits mailing list cfe-commits@lists.

[PATCH] D150446: [analyzer] Check ArraySubscriptExprs in ArrayBoundCheckerV2

NoQ added a comment. > By the way, I'm fed up with the hack that ElementRegion is used for three > separate things ("real" array indexing, casts and pointer arithmetic). To fix > this I'm thinking about introducing a subclass hierarchy where a base class > `ElementLikeRegion` has three subclass

[PATCH] D150446: [analyzer] Check ArraySubscriptExprs in ArrayBoundCheckerV2

donat.nagy added a comment. @NoQ **Re: ElementRegion hacks:** Your suggestion is very convincing, and I agree that while my idea would bring some clarity to some particular issues, overall it would just worsen the "chaotic heap of classes" situation. The only advantage of my solution is that it

[PATCH] D150446: [analyzer] Check ArraySubscriptExprs in ArrayBoundCheckerV2

steakhal requested changes to this revision. steakhal added a comment. This revision now requires changes to proceed. In D150446#4337657 , @donat.nagy wrote: > After some thinking and discussion with @gamesh411 I decided that it'd be > better to replace

[PATCH] D150446: [analyzer] Check ArraySubscriptExprs in ArrayBoundCheckerV2

donat.nagy added a comment. Thanks for the review and the testing! >> [...] What do you think about this design direction? > > Could you elaborate on this part? In short, this review should've been a discourse thread about my "switch to the PreStmt route" plan: I wanted to ask for architectural

[PATCH] D150446: [analyzer] Check ArraySubscriptExprs in ArrayBoundCheckerV2

steakhal added a comment. Okay. I think we are aligned. I'm still uncomfortable sacrificing reports for fast pace development. IMO if that is the goal, then a brand new alpha checker is the way forward. You can be sure that no external users depend on it as its brand new. Unlike with this V2 ch

[PATCH] D150446: [analyzer] Check ArraySubscriptExprs in ArrayBoundCheckerV2

donat.nagy added a comment. Hmm, I agree that a new checker might be better for the situation when we'd sacrifice a significant amount of TPs, but after reconsidering the situation I think that my concerns are mostly theoretical: if the current code reports an issue, then it's probably not too