steakhal added a comment. I'm impressed. Though, I had some nits, please don't take it to heart :)
And consider joining the to the pre-merge beta testing <https://reviews.llvm.org/project/view/78> project to benefit from buildbots and much more - for free. ================ Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:217 + + const llvm::APSInt sampleValue = getMinValue(); + const bool isUnsigned = sampleValue.isUnsigned(); ---------------- Should we take it as `const ref` to prevent copying? ================ Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:235 + // handle a special case for MIN value + if (isFromMinValue) { + // if [from, to] are [MIN, MAX], then just return the same [MIN, MAX] ---------------- AFAIK in a `RangeSet` the ranges are sorted by the `From` point in ascending order. We should not check it for each range, only for the first. Also, note that small and flat loops are more readable. Consider using //pure// lambdas to reduce the body of the loop and of course to bind algorithms to names. ================ Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:572-573 if (const RangeSet *negV = State->get<ConstraintRange>(negSym)) { - // Unsigned range set cannot be negated, unless it is [0, 0]. - if ((negV->getConcreteValue() && - (*negV->getConcreteValue() == 0)) || + if (T->isUnsignedIntegerOrEnumerationType() || T->isSignedIntegerOrEnumerationType()) return negV; ---------------- Couldn't we simplify the disjunction to single `T->isEnumerationType()` or to something similar? ================ Comment at: clang/test/Analysis/constraint_manager_negate_difference.c:114-118 +void negated_unsigned_range(unsigned x, unsigned y) { + clang_analyzer_eval(x - y != 0); // expected-warning{{FALSE}} expected-warning{{TRUE}} + clang_analyzer_eval(y - x != 0); // expected-warning{{FALSE}} expected-warning{{TRUE}} + clang_analyzer_eval(x - y != 0); // expected-warning{{FALSE}} expected-warning{{TRUE}} +} ---------------- What does this test case demonstrate? Could you elaborate on that? Why do you evaluate the `x - y != 0` here twice? ================ Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:23 +template <typename T> struct TestCase { + RangeSet original; + RangeSet expected; ---------------- According to the [LLVM coding style](https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly) we should use UpperCamelCase for variable names for new code. Note that the Analyzer Core was written in a different style, we should follow that there instead (at least IMO). ================ Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:27-28 + TestCase(BasicValueFactory &BVF, RangeSet::Factory &F, + const std::initializer_list<T> &originalList, + const std::initializer_list<T> &expectedList) + : original(createRangeSetFromList(BVF, F, originalList)), ---------------- AFAIK since `std::initializer_list` is just two pointers we should take it by value. ================ Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:51 + // init block + DiagnosticsEngine Diag{new DiagnosticIDs, new DiagnosticOptions}; + FileSystemOptions FileSystemOpts; ---------------- Generally, `new expressions` are a code smell. We should use something like an `std::make_unique` to prevent memory leaks on exceptions. Though, I'm not sure if there is a similar function for `llvm::IntrusiveRefCntPtr<T>`s. ================ Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:66 + template <typename T> void checkNegate() { + using type = T; + ---------------- To be honest, I'm not sure if `type` is more descriptive than `T`. ================ Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:68 + + // use next values of the range {MIN, A, B, MID, C, D, MAX} + ---------------- AFAIK full sentences are required for comments. https://llvm.org/docs/CodingStandards.html#commenting ================ Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:113-120 + // c.original.print(llvm::dbgs()); + // llvm::dbgs() << " => "; + // c.expected.print(llvm::dbgs()); + // llvm::dbgs() << " => "; + // negatedFromOriginal.print(llvm::dbgs()); + // llvm::dbgs() << " => "; + // negatedBackward.print(llvm::dbgs()); ---------------- Should we keep this? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77802/new/ https://reviews.llvm.org/D77802 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
