On 22/05/2014 03:29, Chandler Carruth wrote:
On Wed, May 21, 2014 at 6:25 PM, Richard Smith <[email protected]
<mailto:[email protected]>> wrote:
On Wed, May 21, 2014 at 5:21 PM, Alp Toker <[email protected]
<mailto:[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]> <mailto:[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.
It's concerning that we lack authoritative data on something as
fundamental as authorship/review/attribution, let alone how many
incidents there have been.
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.
It's actually not OK that we have several commits with unclear
reviewership, going back weeks or months.
When a reviewer is named in the commit log, that's an explicit way of
saying "This patch has been looked over by X" and communicating that the
patch doesn't strictly need further post-commit review.
You usually name reviewers when a change is unconventional/unexpected,
say, breaking a stable API for some reason that would normally raise
questions. So it does matter somewhat if the information has been
misleading reviewers.
Now we're in a situation where changes like that have landed which
haven't necessarily been looked over.
It's OK to disable the tool until it gets fixed, but that doesn't mean
we don't still need to look back and at least try to understand if
patches have been going in through the side entrance.
Alp.
(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] <mailto:[email protected]>
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
--
http://www.nuanti.com
the browser experts
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
