xazax.hun added inline comments.
================ Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:31 +/// every other element is a field, and the element that precedes it is the +/// object that contains it. +class FieldChainInfo { ---------------- As far as I understand, for every operation, the only relevant contribution of the non-last elements in the chain is the diagnostic message. I wonder if you would build a string instead of a FieldRegion chain and only store the last FieldRegion would make this more explicit in the code. ================ Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:187 + + if (isCalledByConstructor(Context)) + return; ---------------- Szelethus wrote: > whisperity wrote: > > I think somewhere there should be a bit of reasoning why exactly these > > cases are ignored by the checker. > At this function's declaration I have some comments describing what this does > and why it does it. Did you find it insufficient? I am a bit surprised why these errors are not deduplicated by the analyzer. Maybe it would worth some investigation? ================ Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:272 +// If Chain's value is None, that means that this function was called to +// determine whether a union has any initialized fields. In this case we're not +// collecting fields, we'd only like to know whether the value contained in ---------------- I find the documentation and the name of the function below misleading. `isNonUnionUninit` tells me this function is not supposed to be called for unions but the documentation suggests otherwise. ================ Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:283 + bool ContainsUninitField = false; + Optional<FieldChainInfo> LocalChain = Chain; + ---------------- Why do you copy here explicitly instead of taking the `Chain` argument by value? ================ Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:292 + if (LocalChain) + LocalChain->push_back(FR); + if (isNonUnionUninit(FR, LocalChain)) ---------------- Don't you need to pop somewhere? https://reviews.llvm.org/D45532 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits