[Ktechlab-devel] Codecheck. =P

[P Zoltan]

On Tue, 29 Sep 2009 20:56:02 +0200, Alan Grimes <agrimes at speakeasy.net>  
wrote:

> I followed my own advice and loaded the latest fixmes... Here's an
> interesting one in a function in code that I designed, wrote, and
> contributed.
>
> Zoltan thinks I have a bug here so let's see if we can explicate the
> code....
>
> // ####################################
> // behaves oddly but doesn't crash if m_a == m_b
> bool QuickMatrix::partialScaleAndAdd(CUI m_a, CUI m_b, const double
> scalor) {
>
> 	if(m_a >= m || m_b >= m) return false;
>
> Here we have a basic sanity check. I tried to make the interface to my
> class as "safe" as possible so it will not raise too much of a stink if
> it is given silly parameters. Notice that the parameters are specified
> as unsigned so that we only need to check the upper end of the range.


  Input validation is always good. I'd also print some message about the  
out of range parameters, not just fail silently. This way bugs cand be  
easily found, if they appear.


>
> 	const double *arow = values[m_a];
> 	double *brow = values[m_b];
>
> When you want to run a loop fast, it is important to avoid repeating as
> many computations as possible, so therefore we do our row
> de-refferencing once before we enter our loop.
>
>
> // iterate over n - m_a columns.
> 	for(unsigned int j = m_a; j < n; j++) // FIXME BUG: j is on X coord. ,
> m_a is on Y, but they are compared. WTF?
> 		brow[j] += arow[j] * scalor;
>
> So, really, what the fuck is going on here? Lets look at this function's
> slightly more general cousin:
>
> ### FROM OTHER FUNCTION:
> 	for(unsigned int j = from; j < n; j++)
> 		brow[j] += arow[j] * scalor;
> #####
>
> This function is almost exactly the same except that there's an
> additional parameter, "from". So the code that upset Zoltan so greatly,
> was simply starting from the diagonal. This is a common requirement for
> things such as Gausian elimination where the lower triangular part of
> the matrix is zero (or being set to zero). So therefore we don't want to
> do any more computations on that part.


  This makes it clear that there's no bug :)

  I just looked at the facts that m_a is on one axis, n is on the other, so  
something is not right. And also skipped that short comment at the front  
of the for loop.

  I'd like to ask you now to write a documentation comment block in the  
front of the method declaration in qmatrix.h :D This way no confusion will  
be made again; for examples, see the other documentation comments in the  
file.

>
> So why, then, did I bother to make a separate function? Because you
> could call "partialSAF(a, a, b);" and it'll do the same thing. Well the
> idea was to have it suitable for use in an inner loop where every cycle
> is precious, so here we spare the stack, registers, and cycles for
> processing that second parameter because, in a very common case, its
> redundant.
>
>
> 	return true;
> }
> // ########################################
>