[PATCH] D86870: [analyzer] Add more tests for ArrayBoundCheckerV2

[Balázs Benics via Phabricator via cfe-commits]

steakhal added inline comments.


================
Comment at: clang/test/Analysis/out-of-bounds-false-positive.c:34
+
+void symbolic_uint_and_int0(unsigned len) {
+  (void)a[len + 1]; // no-warning
----------------
martong wrote:
> steakhal wrote:
> > martong wrote:
> > > Hmm, this seems to be quite redundant with the `size_t` tests. Why is it not enough to have test for one unsigned type?
> > > Are you trying to check for overflow errors? Then I'd expect to have indexes around UINT_MAX and so on. 
> > > 
> > > Same comment applies to the tests with the signed types.
> > In the current implementation - and in any implementation of the checker logic will have to deal with //integral-promotion// during the //simplification// of the //array indexer expression// and the given //extent//.
> > All of these can have different signess and bitwidth which makes the implementation quite tricky.
> > 
> > In fact, this resulted in the bug, which this patch-stack aims to fix.
> > I'm gonna highlight the related parts in the refactoring patch if you think it helps.
> Okay, makes sense. 
> 
> It's just very painful to have code repetitions, even in the test code. In gtest unittests we can have tests with type parameters to avoid this kind of repetition. But I guess, there is no way to get rid of this repetition in lit tests.
You can imagine duplicating all of this several times, since the constant in the subscript expression could also have different types, such as: `unsigned`, `long`, `unsigned long`, etc. Potentially uncovering bugs.
However, I was reluctant to add those as well :|

I could introduce a macro, to expand these - but IMO macros in tests ehh. probably reduces readability.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86870/new/

https://reviews.llvm.org/D86870