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

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.

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.

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

From: llvmdev-bounces@cs.uiuc.edu [mailto:llvmdev-bounces@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"

> 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.