This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: GNU Tools Cauldron 2017 follow up: "Reviewed-by" etc.


On Thu, 21 Sep 2017 12:18:39 -0600, Carlos O'Donell <> wrote:
> On 09/21/2017 11:56 AM, Richard Biener wrote:
> > On Thu, 21 Sep 2017 11:38:29 -0600, Carlos O'Donell <> wrote:
> > > On 09/21/2017 10:50 AM, Thomas Schwinge wrote:
> > > > So my question is, if I've gotten a patch reviewed by someone who is not
> > > > yet ;-) familiar with that new process, and I nevertheless want to
> > > > acknowledge their time invested in review by putting "Reviewed-by" into
> > > > the commit log, is it fine to do that if the reviewer just answered with
> > > > "OK" (or similar) instead of an explicit "Reviewed-by: NAME <EMAIL>"
> > > > statement?
> > > You should instead ask the author to give their "Reviewed-by:" and point
> > > out what the Reviewed-by statement means.
> > > 
> > > > That is, is it fine to assume that our current patch review's standard
> > > > "OK" (or similar) answer matches the more formal "Reviewer's statement of
> > > > oversight"?
> > > 
> > > Not yet.
> > 
> > I think given an OK from an official reviewer entitles you to commit
> > it indeed IS matching the formal statement. It better does...

I certainly understand your rationale, and do agree to that -- yet, I can
see how somebody might get offended if turning a casual "OK" into a
formal "Reviewed-by: NAME <EMAIL>", so...

> Isn't it better to be explicit about this; rather than assuming?

..., yeah, that makes sense.

Anyway: aside from starting to use them, we should also document such new
processes, so we might do it as follows, where I had the idea that the
*submitter* 'should encourage the reviewer to "earn" this

Gerald, OK to commit?  If approving this patch, please respond with
"Reviewed-by: NAME <EMAIL>" so that your effort will be recorded.  See
<>.  There you go.  ;-)

Index: htdocs/contribute.html
RCS file: /cvs/gcc/wwwdocs/htdocs/contribute.html,v
retrieving revision 1.87
diff -u -p -r1.87 contribute.html
--- htdocs/contribute.html	9 Apr 2015 21:49:31 -0000	1.87
+++ htdocs/contribute.html	22 Sep 2017 18:20:04 -0000
@@ -23,7 +23,7 @@ contributions must meet:</p>
 <li><a href="#testing">Testing Patches</a></li>
 <li><a href="#docchanges">Documentation Changes</a></li>
 <li><a href="#webchanges">Web Site Changes</a></li>
-<li><a href="#patches">Submitting Patches</a></li>
+<li><a href="#patches">Preparing Patches</a></li>
 <li><a href="#announce">Announcing Changes (to our Users)</a></li>
@@ -164,7 +164,7 @@ file" mode of the validator.</p>
 <p>More <a href="about.html#cvs">about our web pages</a>.</p>
-<h2><a name="patches">Submitting Patches</a></h2>
+<h2><a name="patches">Preparing Patches</a></h2>
 <p>Every patch must have several pieces of information, <em>before</em> we
 can properly evaluate it:</p>
@@ -257,6 +257,71 @@ bzip2ed and uuencoded or encoded as a <c
 acceptable, as long as the ChangeLog is still posted as plain text.
+<!-- (Eventually) referenced from many places.  -->
+<h3><a name="patches-review">Acknowledge Patch Review</a></h3>
+<p>Patch review often is a time-consuming effort.  It is appreciated to
+  acknowledge this in the commit log.  We are adapting
+  the <code>Reviewed-by:</code> tag as established by the Linux kernel patch
+  review process.</p>
+<p>As this is not yet an established process in GCC, you, as the submitter,
+  should encourage the reviewer to "earn" this acknowledgement.  For example,
+  include the following in your patch submission:</p>
+  <p>If approving this patch, please respond with "Reviewed-by: NAME
+    &lt;EMAIL&gt;" so that your effort will be recorded.  See
+    &lt;;.
+  </p>
+<p>For reference, reproduced from
+  the <a href="";>Linux
+  kernel 4.13's <code>Documentation/process/submitting-patches.rst</code></a>:
+<blockquote cite="";>
+  <p><em>Reviewed-by:</em> [...] indicates that the patch has been reviewed
+    and found acceptable according to the Reviewer's Statement:<br>
+<strong>Reviewer's statement of oversight</strong><br>
+By offering my <em>Reviewed-by:</em> tag, I state that:<br>
+	 (a) I have carried out a technical review of this patch to
+	     evaluate its appropriateness and readiness for inclusion [...].
+	 (b) Any problems, concerns, or questions relating to the patch
+	     have been communicated back to the submitter.  I am satisfied
+	     with the submitter's response to my comments.
+	 (c) While there may be things that could be improved with this
+	     submission, I believe that it is, at this time, (1) a
+	     worthwhile modification [...], and (2) free of known
+	     issues which would argue against its inclusion.
+	 (d) While I have reviewed the patch and believe it to be sound, I
+	     do not (unless explicitly stated elsewhere) make any
+	     warranties or guarantees that it will achieve its stated
+	     purpose or function properly in any given situation.
+A <em>Reviewed-by:</em> tag is a statement of opinion that the patch is an
+appropriate modification [...] without any remaining serious
+technical issues.  Any interested reviewer (who has done the work) can
+offer a <em>Reviewed-by:</em> tag for a patch.  This tag serves to give credit to
+reviewers and to inform maintainers of the degree of review which has been
+done on the patch.  <em>Reviewed-by:</em> tags, when supplied by reviewers known to
+understand the subject area and to perform thorough reviews, will normally
+increase the likelihood of your patch getting [...] [approved].
+<h3>Submitting Patches</a></h3>
 <p>When you have all these pieces, bundle them up in a mail message and
 send it to <a href="lists.html">the appropriate mailing list(s)</a>.
 (Patches will go to one or more lists depending on what you are

(I have not yet spent much time on verifying the HTML, or formatting


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]