[MPlayer-dev-eng] Re: PATCH [0/12] CoreAVC support (Take 3)

[Reimar Döffinger]

Hello,
On Thu, Mar 01, 2007 at 11:09:28AM -0800, Alan Nisota wrote:
> The Wanderer wrote:
> >>Not sure what cosmetics you mean.  The only 'cosmetics' are a 2 line
> >>comment explaining WTF is going on.
> >
> >The cosmetics he's referring to are the reindentation of several lines -
> >most specifically the "if (result)" block, but possibly also the "
> >result = This->m_pOutputPin->vt->ReceiveConnection(This->m_pOutputPin,"
> >line in the first section. MPlayer policy dictates that such changes -
> >that is, speaking approximately, changes which do not modify the content
> >of a line as far as the compiler output is concerned - be made in
> >separate patches from any changes which have functional effect.
> But I moved that section inside a new 'if' statement, so it isn't 
> behaving the same way it did before, and thus I would say re-indentation 
> makes sense as part of this patch.  If mplayer policy is to not include 
> this...well, that would make the patch harder to review in my opinion, 
> not easier.  But if this is really the policy, let me know.

It is and has always been. And since it makes the patch larger it does
make reviewing it harder. Reviewing is mostly about regressions, and
thus seeing what changed and what did not is very important.

Greetings,
Reimar Döffinger