=?utf-8?q?Donát?= Nagy <donat.n...@ericsson.com>, =?utf-8?q?Donát?= Nagy <donat.n...@ericsson.com> Message-ID: In-Reply-To: <llvm/llvm-project/pull/70...@github.com>
https://github.com/steakhal approved this pull request. I only had a couple `std::move`s missing, an FYI comment, and one question about the diagnostics in the tests. Even in the current state, I think it's a good baby step in improving the status quo. Thank you for pushing this forward! --- 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. Note that I'm not raising these because I feel we have to adjust, but rather because I find the reviews experience on GitHub really poor in general. Comments disappear, comments don't show up for the patch author, only if they look at the right pane like "Conversations" and "Files changed" - yes, not all comments are present on each -.- And I've been bitten by it as a patch author many many times, and expectations around "reacting on comments" helped to mitigate this pain to some degree. Note that this is only my personal preference, and if I'm not mistaken, there is no legitimized consensus around the project. At least, last time I checked the following relevant thread around GitHub review workflows: https://discourse.llvm.org/t/rfc-github-pr-resolve-conversation-button/73178/ https://github.com/llvm/llvm-project/pull/70056 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
