Szelethus added a comment. I really like the high level idea proposed by this patch, and the test files make me believe that its correct as well! I'm really not familiar around this part of the code, so if its okay, I'll take my time to do the usual find references, inserting unreachables/asserts to gain a better understanding before the green checkmark.
================ Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:157 ClusterBindings::Factory *CBFactory; + bool IsMainAnalysis; ---------------- NoQ wrote: > We could have put this flag into `RegionStoreManager` instead - after all, it > doesn't change for the whole duration of the analysis. It would have made it > easier to handle (avoid weird bitwise logic) and probably cheaper, but it'll > make the manager needlessly stateful (which wouldn't really hurt at all right > now, just looks ugly). Could you please add some comments to this? We'll forget about this name I fear in no time. ================ Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:215 + asImmutableMap().getRootWithoutRetain(), IsMainAnalysis}; + return (Store)Ptr.getOpaqueValue(); + } ---------------- To me personally, `reinterpret_cast` is a louder shout about having to use statically unverifiable casts that may be dangerous. I don't insist on it however. ================ Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:394 + /// getInitialStore - Creates the Store that correctly represents + /// memory contents before initiating analysis of the given top-level ---------------- https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments > Don’t duplicate function or class name at the beginning of the comment. Though I'll concede to the argument that we're keeping consistency. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65361/new/ https://reviews.llvm.org/D65361 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
