On Wed, May 21, 2014 at 6:25 PM, Richard Smith <[email protected]>wrote:
> On Wed, May 21, 2014 at 5:21 PM, Alp Toker <[email protected]> wrote: > >> >> On 22/05/2014 03:04, Chandler Carruth wrote: >> >> >>> On Wed, May 21, 2014 at 5:38 PM, Alp Toker <[email protected] <mailto: >>> [email protected]>> 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 >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
