[cfe-commits] r163013 - in /cfe/trunk: include/clang-c/ include/clang/AST/ include/clang/Basic/ include/clang/Sema/ lib/AST/ lib/CodeGen/ lib/Parse/ lib/Sema/ lib/Serialization/ test/Parser/ tools/libclang/

[John McCall]

On Sep 4, 2012, at 9:42 AM, Chandler Carruth wrote:
> On Fri, Aug 31, 2012 at 2:45 PM, Joao Matos <ripzonetriton at gmail.com> wrote:
> Author: triton
> Date: Fri Aug 31 13:45:21 2012
> New Revision: 163013
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=163013&view=rev
> Log:
> Improved MSVC __interface support by adding first class support for it, instead of aliasing to "struct" which had some incorrect behaviour. Patch by David Robins.
> 
> For the record, I do not think this should have been committed. David mailed the patch and you reviewed it, but both of you are in the 'commit-after-approval' group as far as I'm aware. This is clearly not an obvious patch, it as a huge extension to the Clang AST.
> 
> John McCall reviewed previous versions of this patch and suggested the changes that led to the current form. Why didn't you wait until he approved it? Was there some email that didn't make it to the list approving the patch? (I know that email was getting dropped with earlier phases of the review for that patch...)
> 
> I don't think we can just revert this though because so many patches have gone in behind it.
> 
> I can't even post-commit review this because several files have had *all* lines changed due to your client thrashing line endings!!! This is really terrible. I know we already discussed this some, but let me re-iterate: you must not submit patches with thrashed line endings like this.

A whitespace-insensitive diff will work, although of course that's not the default for most tools, and it's definitely going to complicate git blames forever on these files.

> John, thoughts on how to handle this? How can you effectively review it?
> 
> I can offer to back out the entire sequence of patches and essentially return us to before this patch went in, and then perhaps we can get meaningful diffs that you can review?

Ugh.  Unfortunately, that will just make naive tools say that *you* made all the changes (only due to svn's interference — IIRC pure git can preserve the authorship / intent history, but...), which is actively worse.  I think we just live with it.

Joao, I'm sure it's frustrating to not have things reviewed as quickly as you'd like, but committing without approval is not a way to achieve this.  You pushed the envelope a bit before;  if you continue this pattern, we may have to revoke your commit privileges, which obviously we'd prefer not to do.

I'll try to review this quickly.

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120904/9fd36147/attachment.html>