[llvm] r210414 - Do materialize for floating point

Daniel Sanders Daniel.Sanders at imgtec.com
Sun Jun 8 14:01:49 PDT 2014


> From: Chandler Carruth [mailto:chandlerc at google.com] 
> Sent: 08 June 2014 19:43
> To: Daniel Sanders
> Cc: Alp Toker; James Molloy; Reed Kotler; llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm] r210414 - Do materialize for floating point
>
> On Sun, Jun 8, 2014 at 11:26 AM, Daniel Sanders <Daniel.Sanders at imgtec.com> wrote:
> > > One other misconception that I need to clear up is that our pre-commit
> > > reviews in Phabricator are actually intended for public review. It is
> > > intended to create public-accessible record that a review happened
> > > along with the content of that review. We then commit under the 'you
> > > can commit to code you maintain without pre-commit review' rule (rule
> > > 3 of
> > > http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access). As
> > > far as the LLVM community is concerned, it is unreviewed at this point
> > > but it is expected that the availability of the pre-commit review
> > > makes most post-commit reviews trivial. Previously, we were trying to
> > > do this kind of thing with our public-accessible-yet-internal bugzilla
> > > but even though that system created a review that was publicly
> > > visible, the LLVM community had no way of finding the review and
> > > therefore it effectively did not exist.
> > >
> >
> > So, that workflow is equivalent to reviewing on the office whiteboard, then
> > sharing a snapshot of the whiteboard in the commit log, right?
> Yes, that's a pretty good analogy.
>
> > I personally think that workflow you describe *is* acceptable to give a little
> > transparency where there's otherwise none, though others here may
> > disagree..
> >
> > The caveat is that those whiteboard snapshots will be naturally subject to
> > *greater* scrutiny than usual. They're the sum of your work, your portfolio
> > and review board all in one and a way of saying "we're skilled enough that
> > we pre-screen our own patches".
> >
> > If you get it right, it's an opportunity to show off the effectiveness of your
> > internal process in coordination with post-commit review. But if that
> > whiteboard is full of unrecognisable scrawls ending in doodles and "LGTM",
> > not so good ;-)
>
> I agree that this seems acceptable.
>
> But here is a question for you: what harm is caused by CC-ing llvm-commits from the
> beginning? If you're going to do a pre-commit review anyways, why not do it on the list?
>
> One reason why I encourage this (specifically, *if* you're going to do a pre-commit review
> at all, doing it entirely on the list) is because it creates a more inclusive and engaged community.
> It also lets community members recognize those providing code review, which is a crazy
> important task and something that is hard to get visibility into already.
>
> -Chandler

These are mostly my opinion with little evidence to back them up but there's a couple fairly weak reasons. The first is that I'd like to avoid the scenario where MIPS patches that need wider review regularly find it hard to attract reviewers because they are difficult to spot among the more mundane patches. To put it another way, my concern is that CC'ing llvm-commits on everything may encourage many people to think of '[mips]' as a spam tag. The second is that CC'ing llvm-commits seems to make people nervous, occasionally to the point of asking for a pre-pre-commit review. Interestingly, the current approach doesn't seem to have the same effect even though the outcome is pretty much the same. It seems that people don't mind admitting mistakes too much, but they like to know what those mistakes are before they reveal them to the world.




More information about the llvm-commits mailing list