[PATCH] D149259: [analyzer][NFC] Use std::optional instead of custom "empty" state

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

steakhal added a comment.

In D149259#4315393 <https://reviews.llvm.org/D149259#4315393>, @donat.nagy wrote:

> @steakhal Could you review this change when you have some time for it?

I didn't forget about this one. It's in progress, but it's difficult to squeeze this in. The ETA is probably tomorrow.

> I'm planning to stabilize and de-alpha this checker with a series of incremental changes; and it'd be good have this commit and the unrelated tweak https://reviews.llvm.org/D149460 merged before I start working on the next steps.

Good luck with moving this out. It's gonna be bumpy.

> My plans for the near future:
>
> - Replace the prototype-level error reporting with real, detailed, useful messages. Implementing this may require some code reorganization to preserve and pass around the details information.

IMO this is the most important step, yes.

> - Perform an early return when there is no real indexing (there are no ElementRegion layers in the load operation).
> - Reconsider the handling of multidimensional arrays: the current code (in fact the function `computeOffset` that I'm refactoring there) says that `arr[0][10]` is a valid way to index `int arr[4][5]` which not what the users would expect (in this simple case, even clang-tidy knows that "array index 10 is past the end of the array", but ArrayBoundsV2 implements complex logic to compare the index with the whole memory region allocated for the array). I'm leaning towards switching to "normal" behavior here.

In fact, I believe it's UB to index an `int arr[4][5]` multi-dimensional array like `arr[0][10]`.

> - Report situations like `struct foo {int field;} arr[5]; arr[10].field = 123;` -- the current implementation doesn't "see" these because this is writing a FieldRegion (which happens to be inside an ElementRegion, but the checker doesn't look for that that).

+1

> I'm also open to feedback/suggestions connected to these plans. It's likely that the code added by the current NFC commit will be rewritten by the followup changes, but I think it's still useful to merge it, because it documents the switch from the old implementation.

I'm looking forward to these change. Count me for the reviews.
Also, note that this is in production here, so it will take me time to verify the patches. So, I'd probably prefer to have a patch stack, and do the evaluation for some selected patches - basically bundling the stack into a few (1-2) checkpoints.
It would mean that the reviews would be done for each revision, but the verification (prior to landing anything) would be done at the selected stages.
WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149259