steakhal added a subscriber: tomasz-kaminski-sonarsource. steakhal added a comment.
In D148355#4279866 <https://reviews.llvm.org/D148355#4279866>, @donat.nagy wrote: > @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)? Right now I don't have the bandwidth for upstreaming our version, and it seems like you already picked up the subject, so it's probably less intrusive for you. But I should speak for my self I know. So, you can update this revision to our version, or modify or inspire by our version. Anything you want. The only thing is, if the patch, in the end, is sufficiently similar to ours, I think we should add credit to the original author, who is I think @tomasz-kaminski-sonarsource (`Tomasz Kamiński <tomasz.kamin...@sonarsource.com>`). I can help to review and do measurements at scale but I don't think I can contribute in other ways. And on the note of doing refinement followup patches. If those are NFC (non-function changes), then those should be separated from semantic changes for sure. Pure NFC changes make validation much easier, equally so the review process. If not, then it's up to you how you think it's the easiest to review the patch set. 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
