[LLVMdev] [llvm] r210424 - Revert "Do materialize for floating point"

[Daniel Sanders]

> -----Original Message-----
> From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at cs.uiuc.edu]
> On Behalf Of Rafael Espíndola
> Sent: 08 June 2014 15:03
> To: Reed Kotler
> Cc: LLVM Developers Mailing List
> Subject: Re: [LLVMdev] [llvm] r210424 - Revert "Do materialize for floating
> point"
> 
> On 8 June 2014 05:43, Reed Kotler <rkotler at mips.com> wrote:
> > Why are you reverting patches for any area that you have no
> > authorization for ?
> >
> > No build was broken. This patch is fine.
> >
> > I am authorized to check in to the Mips area and Daniel is the
> > maintainer for that area.
> 
> Hi Reed,
> 
> I hadn't noticed this patch until now, so this is mostly about protocol, not my
> opinion about the patch.
> 
> Code ownership in llvm is a statement more about responsibility than actual
> authority. Think of it as owning the code review, not the code proper. It is
> the code owner responsibility to make sure the reviews run correctly for a
> particular section, but there is no special permission for cutting out others
> from the review process or other shortcuts.

I agree. I've clarified this misconception in more detail on the llvm-commits thread but to re-iterate the main point on this list: The developer policy distinguishes between developers who maintain/contributed code and those who don't/didn't to guide decisions on whether on-list pre-commit review is required but that's as far as it goes. There's no concept of authorized maintainers for an area.

> In this particular case it looks like some review feedback was ignored, at least
> in the public list. For example, if the isa x dyn_cast issue is a pattern over all
> of the llvm code base. So if a misuse was noticed during code review, it
> should be handled. If outside of the list it was decided that the original patch
> was actually correct, it is good practice to say something on commit message
> about it.

For what it's worth, it turns out that I'd asked Reed to make a change that he'd already made way back on the 1st of May which confused him. I discovered this yesterday (after replying to the llvm-commits thread) when I went to explain the request in more detail. That said, I agree with the 'understand first, commit later' comments that were raised on the llvm-commits thread. The commit shouldn't have happened until I had clarified the request, discovered that it was already done, and replied to the review.

> Same goes for having a commit message more in line with what we do in
> llvm. Looking at it now I see that it has paragraphs that are not even full
> sentences ("formatting"). What is being formatted? Why is that not an
> independent patch?
> 
> So it does look like we will be better off with the patch reverted and
> recommitted, but then with a better explanation of what is going on and the
> llvm wide issues (isa X dyn_cast, incremental commits, etc) fixed.
> 
> Cheers,
> Rafael
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev