[Lame-dev] Re: Fwd: ping-pong patches, round #3

[Mark T. <mt...@su...>]

ok, go ahead and add them.  

Mark


> >> 
> >> http://www.leidinger.net/lame/ping-pong_no-testcase-change_20001212.patch
> >> is another set of patches (tested with "make test" and something similar
> >> for vbr).
> >> 
> > 
> > This patch has a few good changes, along with adding some insulting
> > comments about my coding, and many possibly dangerious 
> > but surely pointless changes.  
> 
> I just tried to bring back backed out changes which didn't change the
> output.
> BTW.: roberts vbr-mtrh diff does change the output of make test (from
> 595 to 300), but only for me, not for him. Can you please test it too?
> 
> > Here's a quick list:
> > 
> > init_outer_loop:  return code is changed from sum>0 to
> > sum>1e-20.  Unless someone has computed that energy 
> > values < 1e-20 will *never* be quanitzed to non zero values,
> > this is introduce a bug just because someone wanted to pointless
> > changes to a 100% fully working routine that I wrote.
> 
> Look into the for loop. As I understand it, the behavior is the same,
> but someone has replaced 576 conditionals with just one conditional
> (with the assumtion that 576 conditionals + 576 integer adds are slower
> than 576 floating point adds + one conditional).
> 
> > Logic in reservoir.c is changed.  I'm sure no bugs were fixed,
> 
> No, the logic is the same, just the implementation of the logic has
> changed. I didn't just applied the patch, I readded it to the source by
> hand and tried to make sure, it didn't changes the behavior.
> 
> > but by changing this we run the risk of introducing new bugs.
> > New code is no clearer than my original code.  Why change this?
> 
> That's your opinion. After doing the merge by hand and ensuring that it
> hasn't changed the logic, I have to say I like the new one more than the
> old one.
> Yes, the old one worked, but the new one is "more correct", or better:
> it gives more hints about the code flow.
> E.g. in the old code, you didn't know if the initial value of
> gfc->ResvMax can be set at another point in the code, with the new code
> you have the implicit information: "The initial value of gfc->ResvMax
> gets set only here".
> The name of this variable also suggests, that it is a variable which is
> only initialised in this function, but this is speculation, I haven't
> looked at it that closely.
> That's IMHO an improvement, and improvements are good by definition,
> right?
> 
> > The 6 line, bug free ms_convert() is rewritten.  I like the
> > version that I wrote better.  Why would someone else feel
> > the need to change this?
> 
> No, it isn't rewritten, there was just a name change from xr to dst and
> from xr_org to src. This gives more information to the reader than xr
> and xr_org. The move of "FLOAT8 ..." can be an optimization, depending
> on the compiler, but I hope todays compilers know how to optimize this
> on their own. You may flag this as an "cosmetic change", I think it's an
> improvement of the readability of the code.
> 
> > tables.c, psymodel.c:  fills the code with more #define KLEMM's,
> > just so he can replace .33333 by 1.0/3.0 in the tables,
> 
> He has additional changes for this piece of code, but because you didn't
> want to change the actual tables, he has to use a copy of them,
> protected by a define. The end result will either be faster (speed),
> cleaner (code) and/or better (quality). He produces an infrastructure on
> which he want's to improve various things. We can't do listening tests
> on not written code, and maintaining a huge diff is very time consuming,
> time which is better spend on improving lame.
> 
> Perhaps Frank jumps in and explaines more about it.
> 
> The code is propperly protected by a define which isn't on by default
> (as requested by you in another message to the mailinglist).
> 
> > and rewrite the simple, clear, bug free, table reading code.
> > This is the worst kind of pointless code motion.
> 
> At the moment it's just code motion, but moving the encoding source into
> libmp3lame/ was also "pointless code motion", at least in the beginnig.
> Now we have a libmp3lame.a (and sometimes in the future libmp3lame.so).
> 
> Can you please have a closer look at those "pointless changes" again?
> 
> Bye,
> Alexander.
> 
> -- 
>             Give a man a fish and you feed him for a day;
>      teach him to use the Net and he won't bother you for weeks.
> 
> http://www.Leidinger.net                       Alexander @ Leidinger.net
>   GPG fingerprint = 7423 F3E6 3A7E B334 A9CC  B10A 1F5F 130A A638 6E7E
> 
>