steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land.
Approved with nits. Thanks! ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp:267 +const char *describeErrnoCheckState(errno_modeling::ErrnoCheckState CS) { + assert(CS == errno_modeling::MustNotBeChecked && ---------------- The same applies for these. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp:298 + if (!State) + return State; + return errno_modeling::setErrnoValue(State, C.getLocationContext(), ErrnoSym, ---------------- I would prefer returning `nullptr` explicitly here. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp:304-307 + std::string Message = "Assuming that function '"; + Message += Fn; + Message += "' is successful, in this case the value 'errno' "; + Message += describeErrnoCheckState(MustNotBeChecked); ---------------- I believe you should use `Twine()` for chaining string slices. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h:83 +/// others are not used by the clients. +const char *describeErrnoCheckState(errno_modeling::ErrnoCheckState CS); + ---------------- It seems like you are already within this namespace. You don't need to explicitly qualify this. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:405 protected: - /// Many of the descendant classes use this value. - const errno_modeling::ErrnoCheckState CheckState; - - ErrnoConstraintBase(errno_modeling::ErrnoCheckState CS) : CheckState(CS) {} + ErrnoConstraintBase() {} ---------------- Why don't you `= default` it if the body is actually empty? ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:416 + /// function call. + class FailureErrnoConstraint : public ErrnoConstraintBase { public: ---------------- I find this name much more appealing than it was before! Thanks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131879/new/ https://reviews.llvm.org/D131879 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits