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

[Michael Niedermayer]

On Sat, Mar 28, 2009 at 02:17:39PM +0100, Diego Biurrun wrote:
> 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 think that switching the DCT and renaming should be seperate.
The renaming can be checked by comparing stripped object files the rest not.


> 
> > > 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 benefit is to keep the changes easy to review, some people read
svnlog and some people like skiping things like renames.
Its rather likely some will want to read through the new dct, maybe to
look for bugs or look for similariry due to possible legal consequences.
if they are split its less code to look at.


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

good, thats one part less to check for me

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Asymptotically faster algorithms should always be preferred if you have
asymptotical amounts of data
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090328/11cc555f/attachment.pgp>