[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

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

steakhal added a comment.

It looks good to me. I don't mind that FIXME. That being said, I'm not even sure it's a FIXME.
Check my comment inline about this.



================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1757-1760
+  // FIXME: Nevertheless, we can't do the same for cases, like:
+  //   const char *str = "123"; // literal length is 4
+  //   char c = str[41];        // offset is 41
+  // It should be properly handled before reaching this point.
----------------
martong wrote:
> ASDenysPetrov wrote:
> > ASDenysPetrov wrote:
> > > martong wrote:
> > > > Thanks for updating the patch! However, this `FIXME` makes me worried. Do you think you could pass the `Decl` itself to `getSValFromInitListExpr` in order to be able to check whether the type is a pointer or an array? 
> > > This worried me as well. Currently I can't find a way to get the `Decl` for `SL`.
> > > 
> > We can load this as is for now with mentioning the known issue.
> This might cause some itchy false positives. Perhaps, we could address this in a follow-up patch and then commit them together?
> Currently I can't find a way to get the Decl for SL.
Why do you need a Decl? The SVal's gotta be an `Element{"123",41 S64b,char}` for the example in the comment (*).

(*) With a minor modification: https://godbolt.org/z/7zhGMnf7P
```lang=C++
template <class T> void clang_analyzer_dump(T);
const char * const str = "123";
const char * str2 = "123";
void top() {
  clang_analyzer_dump(&str[41]);  // &Element{"123",41 S64b,char}
  clang_analyzer_dump(&str2[41]); // &Element{SymRegion{reg_$0<const char * str2>},41 S64b,char}
}
```


================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1784-1786
+      const auto Offset = static_cast<uint64_t>(Idx.getExtValue());
+      if (Idx < 0)
         return UndefinedVal();
----------------
I think it would be better to have the check before we calculate the `Offset`. At least that way I find it easier to follow.


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

https://reviews.llvm.org/D107339