r209319 - Sema: Implement DR244

[Richard Smith]

On Wed, May 21, 2014 at 5:21 PM, Alp Toker <alp at nuanti.com> wrote:

>
> On 22/05/2014 03:04, Chandler Carruth wrote:
>
>
>> On Wed, May 21, 2014 at 5:38 PM, Alp Toker <alp at nuanti.com <mailto:
>> alp at nuanti.com>> wrote:
>>
>>     On 21/05/2014 23:19, David Majnemer wrote:
>>
>>         Author: majnemer
>>         Date: Wed May 21 15:19:59 2014
>>         New Revision: 209319
>>
>>         URL: http://llvm.org/viewvc/llvm-project?rev=209319&view=rev
>>         Log:
>>         Sema: Implement DR244
>>
>>         Summary:
>>         Naming the destructor using a typedef-name for the class-name is
>>         well-formed.
>>
>>         This fixes PR19620.
>>
>>         Reviewers: rsmith, doug.gregor
>>
>>
>>     Did Doug participate in review for this patch?
>>
>>
>> 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.
>>
>
> Take a look at the Reviewers line?
>
>
>
>>     Looking through SVN history, I'm seeing an alarming number of
>>     confusing review trails.
>>
>>
>> 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.
>>
>>
>>     If review happened off-list that's fine, but it needs to be stated
>>     clearly because the system works on trust.
>>
>>
>> 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.
>>
>
> I'd expect that off-list review would be mentioned explicitly rather than
> everyone making the implicit assumption that's the case.
>
> In the case of r209319, Doug is mentioned as reviewer but I only see
> Richard's review comments on the list.
>
> That's why I asked for clarification on Doug's participation -- was it
> off-list, or is it a mistake in the commit log?
>
>
>
>>     (In fact, I'm seeing relatively inactive developer names showing
>>     up suspiciously in these "Reviewers" lines while some of the most
>>     active reviewers barely appear at all. Could this be a problem
>>     with Phabricator or some internal system you guys are using?)
>>
>>
>> I'm really not sure what you're worried about here. Again, specific
>> examples?
>>
>
> 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.
>

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.


> (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.)
>
> Alp.
>
>
>
> --
> http://www.nuanti.com
> the browser experts
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140521/cfefddd4/attachment.html>