<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, May 21, 2014 at 6:25 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5">On Wed, May 21, 2014 at 5:21 PM, Alp Toker <span dir="ltr"><<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On 22/05/2014 03:04, Chandler Carruth wrote:<div><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On Wed, May 21, 2014 at 5:38 PM, Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a> <mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>>> wrote:<br>
<br>
On 21/05/2014 23:19, David Majnemer wrote:<br>
<br>
Author: majnemer<br>
Date: Wed May 21 15:19:59 2014<br>
New Revision: 209319<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=209319&view=rev" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project?rev=209319&view=rev</a><br>
Log:<br>
Sema: Implement DR244<br>
<br>
Summary:<br>
Naming the destructor using a typedef-name for the class-name is<br>
well-formed.<br>
<br>
This fixes PR19620.<br>
<br>
Reviewers: rsmith, doug.gregor<br>
<br>
<br>
Did Doug participate in review for this patch?<br>
<br>
<br>
No, Richard Smith did. I see a pretty complete review thread for the DR244 patch in my inbox. I've even checked and none of the emails were lost in the recent email list snafu.<br>
</blockquote>
<br></div>
Take a look at the Reviewers line?<div><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Looking through SVN history, I'm seeing an alarming number of<br>
confusing review trails.<br>
<br>
<br>
Can you point them out specifically? I too keep a pretty close eye on these kinds of things and I'm not seeing any examples.<br>
<br>
<br>
If review happened off-list that's fine, but it needs to be stated<br>
clearly because the system works on trust.<br>
<br>
<br>
I don't think off-list review is fine... It happens some times, for good or bad reasons, and its not the end of the world. But it is definitely not SOP, and I haven't seen any evidence of it becoming more pervasive. If you see it, call it out. When patches merit pre-commit review, they should get it from the whole community.<br>
</blockquote>
<br></div>
I'd expect that off-list review would be mentioned explicitly rather than everyone making the implicit assumption that's the case.<br>
<br>
In the case of r209319, Doug is mentioned as reviewer but I only see Richard's review comments on the list.<br>
<br>
That's why I asked for clarification on Doug's participation -- was it off-list, or is it a mistake in the commit log?<div><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
(In fact, I'm seeing relatively inactive developer names showing<br>
up suspiciously in these "Reviewers" lines while some of the most<br>
active reviewers barely appear at all. Could this be a problem<br>
with Phabricator or some internal system you guys are using?)<br>
<br>
<br>
I'm really not sure what you're worried about here. Again, specific examples?<br>
</blockquote>
<br></div>
So I want to know if Doug was involved with review of this change as stated in the commit log. That's a good place to start.<br></blockquote><div><br></div></div></div><div>The "Reviewers" line that (IIUC) arcanist adds lists the reviewers in Phab, which includes Doug, and doesn't indicate that anyone mentioned actually reviewed anything. This is a list of people who were asked to review, not a list of people who did so.</div>
</div></div></div></blockquote><div><br></div><div>And considering that not everyone (not even most) use arcanist to submit, this hasn't really even come up as an interesting bit of metadata, much less an authoritative one. I don't know whether anyone would want to actually fix arcanist to not have this behavior -- the tool is not easy to hack IIUC. =/ I think the only useful thing to include would be a link to the review.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">
(No sleight towards David, we're working together on plenty of stuff day to day. It's just good practice to keep a reasonable paper trail for authorship / reviewership and ask questions when things look out of place.)<span><font color="#888888"><br>
<br>
Alp.</font></span></div><div><div><div class=""><br>
<br>
<br>
-- <br>
<a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<br></div><div class="">
______________________________<u></u>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/cfe-commits</a><br>
</div></div></div></blockquote></div><br></div></div>
</blockquote></div><br></div></div>