MTC added a comment.
Sorry for the long delay, I have just finished my holiday.
Thanks a lot for the review, I will fix the typo in next update.
================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2100
+ std::tie(StateNullChar, StateNonNullChar) =
+ assumeZero(C, State, CharVal, Ctx.UnsignedCharTy);
+
----------------
a.sidorin wrote:
> I think we can use the argument type here (which is int).
> One more question. Could we use `SVal::isZeroConstant()` here instead? Do we
> need the path-sensitivity for the value or we can just check if the value is
> a constant zero?
Yea, it's should be int.
`SVal::isZeroConstant()` is enough here, thanks!
One question that has nothing to do with the patch, I would like to know if
there is a standard to determine whether there is a need for state splitting.
================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2127
+ /*IsSourceBuffer*/ false, Size);
+ }
+
----------------
a.sidorin wrote:
> Could you please factor this chunk to a function? It makes the method too big.
will do.
================
Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:148
+ ProgramStateRef NewState = makeWithStore(NewStore);
+ return Mgr.getOwningEngine()
+ ? Mgr.getOwningEngine()->processRegionChange(NewState, R, LCtx)
----------------
a.sidorin wrote:
> Do clients of `overwriteRegion` really need to call checkers?
It is possible that my understanding of the engine is not enough, I think we
need to call `processRangeChange()` for memory change. `memset()` will
overwrite the memory region, so I think there is a need to call this API.
What do you think?
================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1718
- llvm_unreachable("Unknown default value");
+ return val;
}
----------------
a.sidorin wrote:
> Could you please explain what is the reason of this change? Do we get new
> value kinds?
`memset()` can bind anything to the region with `default binding`, if there is
no such change, it will trigger `llvm_unreachable()`.
I don't think much about it, just put `return val;` here. Any suggestions?
================
Comment at: lib/StaticAnalyzer/Core/Store.cpp:72
+StoreRef StoreManager::OverwriteRegion(Store store, const MemRegion *R, SVal
V) {
+ return StoreRef(store, *this);
----------------
a.sidorin wrote:
> Could we make this method pure virtual?
To be honest, I implemented this function according to `BindDefault()`. And I
also think pure virtual is better, thanks.
Repository:
rC Clang
https://reviews.llvm.org/D44934
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
