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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
