r209319 - Sema: Implement DR244

[Chandler Carruth]

On Wed, May 21, 2014 at 6:25 PM, Richard Smith <richard at metafoo.co.uk>wrote:

> 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.
>

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.


>
>
>> (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/b300437d/attachment.html>