a.sidorin added a comment. Hi Henry. I had a quick look at the patch, here are some remarks.
================ Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2092 + // 'invalidateRegions()', which will remove the <MemRegion, SVal> pair + // in CStringLength map. So calls 'InvalidateBuffer()' after getting + // old string length and before setting the new string length. ---------------- "So we call"? ================ Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2100 + std::tie(StateNullChar, StateNonNullChar) = + assumeZero(C, State, CharVal, Ctx.UnsignedCharTy); + ---------------- 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? ================ Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2103 + if (StateNullChar && !StateNonNullChar) { + // If the value of the second arguement of 'memset()' is zero, set the + // string length of destination buffer to 0 directly. ---------------- "argument" ================ Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2127 + /*IsSourceBuffer*/ false, Size); + } + ---------------- Could you please factor this chunk to a function? It makes the method too big. ================ Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:148 + ProgramStateRef NewState = makeWithStore(NewStore); + return Mgr.getOwningEngine() + ? Mgr.getOwningEngine()->processRegionChange(NewState, R, LCtx) ---------------- Do clients of `overwriteRegion` really need to call checkers? ================ Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:446 + // default binding. + StoreRef OverwriteRegion(Store store, const MemRegion *R, SVal V) override { + RegionBindingsRef B = getRegionBindings(store); ---------------- LLVM naming conventions say that function names should start with small letter. ================ Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1718 - llvm_unreachable("Unknown default value"); + return val; } ---------------- Could you please explain what is the reason of this change? Do we get new value kinds? ================ Comment at: lib/StaticAnalyzer/Core/Store.cpp:72 +StoreRef StoreManager::OverwriteRegion(Store store, const MemRegion *R, SVal V) { + return StoreRef(store, *this); ---------------- Could we make this method pure virtual? 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