steakhal requested changes to this revision. steakhal added a comment. This revision now requires changes to proceed.
In D112621#3160082 <https://reviews.llvm.org/D112621#3160082>, @manas wrote: > I have made few changes: > > 1. The failed assertions <https://reviews.llvm.org/F19816216> due to > comparison between different types have been fixed by converting all the > Ranges to a given type. This will allow comparisons without throwing errors. > 2. There was also a build error due to `explicit specialization in > non-namespace scope`. This was fixed by @martong previously, but that patch > led to the above mentioned bug. So I used @martong 's patch here to make GCC > happy. > 3. I have added a small check for comparison between different types. > > https://reviews.llvm.org/D106102 differential contains the background story. I'm not quite happy. Next time please make sure that the tests you provide actually exercise the changed code segments. I deleted all the old lines of the `constant-folding.c` test file and placed a `llvm_unreachable()` into `SymbolicRangeInferrer::VisitBinaryOperator<BO_NE>()` and the test did not cause a crash, which is a clear indication that the test does not test the modified code segments. Unless I'm overly anxious I would draw false conclusions about the results of the tests and if I approve we could get crashes and complaints, and reverts. I would kindly ask you to double-check your tests prior to submitting them to review. --- About the technical content of this patch. I would recommend following the same pattern as in the rest of the functions. So, `fillGaps()` then `convert()`. That way you would reduce the time-complexity of your code to `O(1)` from `O(N)`. I would like to ask for a coverage report that shows that all of the branches of the modified code have been exercised by the `constant-folding.c` test file. That being said, I would like to thank you for your effort in fixing this, I'm really looking forward! ================ Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1272-1273 + // (x != y). + if ((ConvertedLHS.getMaxValue() < ConvertedRHS.getMinValue()) || + (ConvertedLHS.getMinValue() > ConvertedRHS.getMaxValue())) { + return getTrueRange(T); ---------------- This and the subsequent similar block could be implemented by a member function of a `RangeSet`. It's highly likely that many of these `VisitBinaryOperator<Op>()` functions could benefit from reusing them in the future if we decide to handle more of them. ================ Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1279-1281 + if ((ConvertedLHS.getMinValue() == ConvertedRHS.getMaxValue()) && + (ConvertedLHS.getMaxValue() == ConvertedRHS.getMaxValue()) && + (ConvertedLHS.getMinValue() == ConvertedRHS.getMinValue())) { ---------------- `ConvertedLHS.getConcreteValue() && ConvertedLHS.getConcreteValue() == ConvertedRHS.getConcreteValue()` ================ Comment at: clang/test/Analysis/constant-folding.c:326 + if (s1 >= -1000 && s1 <= -500 && s2 <= -500 && s2 >= -750) { + // s1: [-1000,-500], s2: [-500,-750] + clang_analyzer_eval((s1 != s2) == 0); // expected-warning{{UNKNOWN}} ---------------- I would assume from `[x, y]` that `x <= y`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112621/new/ https://reviews.llvm.org/D112621 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
