[FFmpeg-devel] [Fwd: Summer of code small task patch]

[Diego Biurrun]

On Sat, Mar 28, 2009 at 02:06:45PM +0100, Michael Niedermayer wrote:
> On Sat, Mar 28, 2009 at 01:52:50PM +0100, Diego Biurrun wrote:
> > On Sat, Mar 28, 2009 at 01:30:39PM +0100, Michael Niedermayer wrote:
> > > On Sat, Mar 28, 2009 at 12:42:46PM +0100, Diego Biurrun wrote:
> > > > On Sat, Mar 28, 2009 at 06:23:14AM +0100, Michael Niedermayer wrote:
> > > > > On Fri, Mar 27, 2009 at 10:18:07PM +0200, Dylan Yudaken wrote:
> > > > > >
> > > > > > --- libavcodec/dct-test.c	(revision 18203)
> > > > > > +++ libavcodec/dct-test.c	(working copy)
> > > > > > @@ -46,9 +46,9 @@
> > > > > >  void *fast_memcpy(void *a, const void *b, size_t c){return memcpy(a,b,c);};
> > > > > >  
> > > > > >  /* reference fdct/idct */
> > > > > > -void fdct(DCTELEM *block);
> > > > > > -void idct(DCTELEM *block);
> > > > > > -void init_fdct(void);
> > > > > > +void ff_ref_fdct(DCTELEM *block);
> > > > > > +void ff_ref_idct(DCTELEM *block);
> > > > > > +void ff_ref_dct_init(void);
> > > > > 
> > > > > renaming things should be a seperate patch
> > > > > actually a patch should either do functional changes or non functional not
> > > > > both
> > > > 
> > > > I think this should be treated differently, he is creating an entirely
> > > > new and independent file.  Adding a renaming step in between is
> > > > pointless extra work.
> > > 
> > > What do you suggest exactly?
> > 
> > Add the new file and apply the renamings in one go.
> 
> Patches should not mix unrelated changes, and even more so for a
> qualification task.

I do not see this as unrelated.  Of course the file could be added first
and then the renamings patch applied along with removing the old file.
But this would only split only logical step into multiple parts.

> > I see no benefit in splitting this.
> 
> This though does not mean others dont.

Which benefit do you see exactly.  Your policy on patch splitting is
often too extreme IMO.  Adding the new reference DCT and hooking it up
in the DCT test is one logical step IMO.

> > The renamings are straightforward and I verified the
> > patch to be correct.
> 
> you verified that the renamings are just renamings?
> or that the whole patch is correct?

The former.

Diego