[PATCH] D38358: [analyzer] Fix autodetection of getSVal()'s type argument.

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin added inline comments.


================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1404
+          // When trying to dereference a void pointer, read the first byte.
+          T = Ctx.CharTy;
+        }
----------------
NoQ wrote:
> dcoughlin wrote:
> > Nit: It seems a bit odd to read the first byte here since (unless I'm misunderstanding) this would never be triggered by actual C semantics, only by a checker. Did you consider just returning UnknownVal() in this case?
> `UnknownVal` seems to be quite similar to assertion failure: in both cases we'd have to force the checker to specify `CharTy` explicitly. But assertions are better because the author of the checker would instantly know that he needs to specify the type, instead of never noticing the problem, or spending a lot of time in the debugger trying to understand why he gets an `UnknownVal`.
 I think having an assertion with a helpful message indicating how the checker author could fix the problem would be great! But you're not triggering an assertion failure here since you're changing T to be CharTy. Instead, you're papering over the problem by making up a fake value that doesn't correspond to the program semantics. If you're going to paper over the the problem and return a value, I think it is far preferable to use UnknownVal. This would signal "we don't know what is going here" rather than just making up a value that is likely wrong.


Repository:
  rL LLVM

https://reviews.llvm.org/D38358