[PATCH] D89995: Make the post-commit review expectations more explicit with respect to revert

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

Overall, I think the policy change proposed here isn't entirely unreasonable, but I do think it needs to be treated as a change of policy. Not everybody is necessarily aware of a 4 year old email thread; heck, I've been working on upstream LLVM and frontends for 5 years and I wasn't aware of this supposed policy where in reality, things weren't actually done according to what's written in the policy documents. If I don't know this after 5 years, how can anybody joining in the last 4 years possibly be aware other than through mere chance. What's written in the documents needs to matter, if only as part of making LLVM a welcoming and inclusive community. Hidden tribal knowledge is the opposite of those ideals.

Back to the proposal itself: The problem I'm seeing (with post-commit review in general, but especially with this change) is that it exacerbates the dysfunction and arbitrariness of the LLVM review process. That's a practical problem because erring too much on the side of stasis blocks progress on dependent work, especially if that dependent work requires collaboration between multiple developers.

I think this needs a solution. That solution can be a separate discussion, but I think it ought to be adopted before making the problem worse. I will try to find some time to write this up.



================
Comment at: llvm/docs/CodeReview.rst:54-55
+patch author, this is inherent also to our post-commit review practices.
+Reverting a patch ensures that design discussions can happen without blocking
+other development; it's entirely possible the patch will end up being reapplied
+essentially as is once concerns have been resolved.
----------------
This is incorrect. Reverting a patch can in practice block other development more than not reverting it. Please just remove this statement.


================
Comment at: llvm/docs/CodeReview.rst:60-62
+actively in the review. In cases where the patch triggered a hardware-specific
+buildbot failure, a community member with access to hardware similar to that on
+the buildbot that the patch previously caused to fail.
----------------
This sentence is missing a verb. Maybe something along the lines of: "In cases where the problem is identified by a buildbot, a community member with access to hardware similar to that on the buildbot is excepted to engage in the review."


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89995/new/

https://reviews.llvm.org/D89995