## ktechlab-devel

 [Ktechlab-devel] Codecheck. =P From: Alan Grimes - 2009-09-29 19:05:50 ```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. 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. 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; } // ######################################## -- New president: Here we go again... Chemistry.com: A total rip-off. Powers are not rights. ```
 Re: [Ktechlab-devel] Codecheck. =P From: P Zoltan - 2009-09-29 19:41:31 ```On Tue, 29 Sep 2009 20:56:02 +0200, Alan Grimes 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; > } > // ######################################## > ```