[PATCH] D99305: [docs] Document our norms around reverts

[Philip Reames via Phabricator via llvm-commits]

reames marked 2 inline comments as done.
reames added a comment.

Response to hubert.reinterpretcast



================
Comment at: llvm/docs/DeveloperPolicy.rst:318-319
+* If you break a buildbot in a way which can't be quickly fixed, please revert.
+* If a test case is reported in the commit thread which demonstrates a problem
+  please revert, and investigate offline.
+* If you receive substantial :ref:`post-commit review <post_commit_review>`
----------------
hubert.reinterpretcast wrote:
> I really do think that at least the comma should be moved. I also suggest changing the ordering to clarify that the "test case" (as opposed to the thread) is the subject of the clause.
I've reread this sentence a couple of times, and don't see your point.  Can you quote the exact change you're suggesting?

(I was never great at formal english grammar so I might be misparsing something.)


================
Comment at: llvm/docs/DeveloperPolicy.rst:350
+* The commit message for the reverting commit should explain why patch
+  is being reverted.  It is also customary to response to the original
+  commit email mentioning the revert.
----------------
hubert.reinterpretcast wrote:
> Fix typo.
Already done.  I noticed this too.  


================
Comment at: llvm/docs/DeveloperPolicy.rst:362
+  requires hardware patch author doesn't have access to, sharp regression in
+  compile time of internal workload, etc..), the reverter is expected to be
+  strongly proactive about working with the patch author to debug and test
----------------
hubert.reinterpretcast wrote:
> Suggest not to have two periods for `etc.`.
This is an ellipsis to indicate the list is not complete.  I believe this is correct usage.  Some sources say it must be three periods (not two), but I see both in regular usage.  


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

https://reviews.llvm.org/D99305