[clang] [analyzer] Improve diagnostics from ArrayBoundCheckerV2 (PR #70056)

via cfe-commits cfe-commits at lists.llvm.org
Thu Nov 2 06:04:36 PDT 2023


=?utf-8?q?Donát?= Nagy <donat.nagy at ericsson.com>,
=?utf-8?q?Donát?= Nagy <donat.nagy at ericsson.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/70056 at github.com>


DonatNagyE wrote:

Thanks for the suggestions; I'll probably upload the next revision tomorrow.

> One remark about the review workflow, I'd prefer if the conversation starter would resolve the conversations. Let me explain why:
> 
> Given the amount of reviewers for CSA, I'd suggest explicitly supporting reviewer experience. Speaking of that, personally I find the following workflow to suite myself the best:
> 
> * All comments needs a reaction, either by explict commenting on it or by putting there a thums-up emoji or something. This helps the author follow which comments were addressed downstream and is pending for submission. An explicit comment is expected for challenging a comment. I prefer emojies, becase they don't send an email to everyone subscribe - unlike for individual comments; and usually I also batch individual commits into a "self-review" as patch author when I reply for the same reason.
> 
> * All comments needs to be reacted before requestion the next round of review from the person who added those comments.
> 
> * The comment should be only marked resolved by the person who raised the concern. This ensures that the comment was adequately solved, thus the issue is no longer relevant.
> 
> * To merge a PR, the PR should have no open conversations. Post-approval comments are also fine and leaves room for further discussions, but the goal is no longer to directly challenge the legitimacy of the patch.

Sounds reasonable, I'll try to follow this process, especially when interacting with you. In the previous round I resolved the conversations because I applied them (mostly verbatim) in the new revision that I uploaded; but I can instead apply a thumbsup emoji if you'd prefer that. 

https://github.com/llvm/llvm-project/pull/70056


More information about the cfe-commits mailing list