r209319 - Sema: Implement DR244

[Alp Toker]

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.

(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