[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/

[David Blaikie]

On Wed, Sep 5, 2012 at 9:43 AM, Matthieu Monrocq
<matthieu.monrocq at gmail.com> wrote:
>
>
> On Tue, Sep 4, 2012 at 8:36 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> On Tue, Sep 4, 2012 at 10:18 AM, John McCall <rjmccall at apple.com> wrote:
>> > 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.
>>
>> (pedantry that seems to warrant being pointed out every time this comes
>> up:
>> git blame -w
>> (& not entirely portable: svn blame -x -w))
>>
>> Though that still leaves our viewvc providing less-than-ideal blame.
>>
>
> Isn't there an option in the viewvc configuration to address that ? I would
> be surprised if the command line offered it and the viewer could not, but it
> may not be implemented :x

Seems there is a server-side config to just make all diffs whitespace
ignorant (hr_ignore_white)
& also someone provided a patch/filed a bug on viewvc to make it a
user-configurable option from the web UI:
http://viewvc.tigris.org/issues/show_bug.cgi?id=492

>
> -- Matthieu
>
>>
>> >
>> > 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.
>> >
>> > _______________________________________________
>> > cfe-commits mailing list
>> > cfe-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> >
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>