donat.nagy added a comment. @steakhal Thanks for the background information!
I didn't know about D86874 <https://reviews.llvm.org/D86874> so I indeed ended up with something very similar to it. I reviewed D88359 <https://reviews.llvm.org/D88359> and I knew that it's a completely general solution of this issue, but I felt that it's too complicated and wanted to create a patch with shorter code than that. I really like the "use zero instead of negative numbers" trick in the SonarSource patch; if you would upload that for a review, I'd strongly support merging it. Another alternative is that I'm working on a new version of my patch, which would eliminate the code duplication between the underflow and overflow checks (by introducing a single function compareValueToThreshold that performs offset simplification when needed, handles the unsigned-vs-negative case, calls evalBinOpNN and invokes state->assume). This would be equivalent to the SonarSource patch (it handles unsigned-vs-negative comparison on "both sides") with the added independent benefit of simplifying the codebase. However, I can also do this code simplification as a separate patch after merging the SonarSource solution for the bug. Which solution would you prefer (upstream the solution used by SonarSource + separate code quality improvement or the combined refactor-and-check-before-evalBinOpNN variant that I could implement)? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148355/new/ https://reviews.llvm.org/D148355 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
