NoQ added a comment.
So, like, I think it's a start. You introduced a single source of truth for
casting `SVal`s and it's good. But i'm worried about our API surface.
================
Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:539
-SVal SValBuilder::evalCast(SVal V, QualType CastTy, QualType OriginalTy) {
- CastTy = Context.getCanonicalType(CastTy);
- OriginalTy = Context.getCanonicalType(OriginalTy);
- if (CastTy == OriginalTy)
+// In case when `OriginalTy.isNull() == true` we cast `V` less accurately.
+SVal SValBuilder::evalCast(SVal V, QualType CastTy,
----------------
NoQ wrote:
> This is one of the most important APIs of `SValBuilder`, alongside
> `evalBinOp()`. The newly added behavior is definitely obscure and needs
> explaining. So i think you should move this comment into the header, turn it
> into a doxygen comment, and elaborate what "less accurately" actually means
> here.
Like, are you sure that the new behavior makes sense at all for call sites that
aren't ex-`CastRetrievedVal`? When, and why, would a regular user use this
functionality?
Maybe instead of obfuscating this functionality as a null pointer we should add
an explicit flag and give it an understandable name? Even if it's `bool
CastRetrievedValBackdoorHackDoNotUse = false` it's still much better than "umm
we kind of expect it to work if you pass null here but we don't really know
how".
================
Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:564
switch (V.getBaseKind()) {
+ default:
+ llvm_unreachable("Unknown SVal kind");
----------------
No, don't add `default:`. All possible cases are currently handled. It breaks
compiler warning about not all cases handled in the switch when new enum cases
are added. Compile-time warnings are better than run-time warnings.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96090/new/
https://reviews.llvm.org/D96090
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
