steakhal added inline comments.
================ Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:412 + class ZeroRelatedErrnoConstraint : public ErrnoConstraintKind { + clang::BinaryOperatorKind Relation; + ---------------- The explicit qualification is not necessary. There are other cases like this. In the constraint manager, we tend to use `Op` for //operator kinds//, it would be nice to use the same taxonomy here. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:417 + errno_modeling::ErrnoCheckState CS) + : ErrnoConstraintKind(CS), Relation(Rel) {} + ---------------- ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:406 + /// It may return empty string if no note tag is to be added. + virtual std::string describe(StringRef FunctionName) const = 0; + ---------------- ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:443-446 + Optional<DefinedOrUnknownSVal> Cond = + SVB.evalBinOpNN(State, Relation, ErrnoSVal, ZeroVal, + SVB.getConditionType()) + .getAs<DefinedOrUnknownSVal>(); ---------------- Just use `.castAs<NonLoc>()`. You will assert it anyway, and I find this much more readable. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:454-460 + std::string Result = "Assuming that function '"; + Result.append(FunctionName.str()); + Result.append("' fails, in this case the value 'errno' becomes "); + Result.append(BinaryOperator::getOpcodeStr(Relation).str()); + Result.append(" 0 and "); + Result.append(describeErrnoCheckState(CheckState)); + return Result; ---------------- I believe you should use `Twine` for this. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1028 + std::string Note = Case.getErrnoConstraint().describe( + D ? D->getNameAsString() : "<unknown>"); + if (Note.empty()) ---------------- What is `<unknown>` in this context? Please add a test for it or remove this branch. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1037-1042 + [Node, Note]() -> std::string { + // Don't emit "Assuming..." note when we ended up + // knowing in advance which branch is taken. + if (Node->succ_size() == 1) + return ""; + return Note.str(); ---------------- Why did you change these? ================ Comment at: clang/test/Analysis/errno-stdlibraryfunctions.c:20-24 +void test2() { + if (access("path", 0) == -1) { + if (errno != 0) { } + } +} ---------------- ================ Comment at: clang/test/Analysis/errno-stdlibraryfunctions.c:28 + if (access("path", 0) != -1) { + // expected-note@-1{{Assuming that function 'access' is successful, in this case the value 'errno' may be undefined after the call and should not be used}} + if (errno != 0) { } ---------------- Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125400/new/ https://reviews.llvm.org/D125400 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits