leading newline

I personally prefer that, just looks better to me. But if you insist :)

i would prefer if this was a single test that checks all 81 test cases and writes "lcm(x,y) = lcm(y,x) for all x and y in [-4,4]". that would make the post window output cleaner.

yes, this is true. How would you do the test so that when some of them fail you know which ones? I ask specifically because brian convincingly argued that each test should have a smaller scope, so that it is easier to analyze.

What about setting UnitTest.reportPasses = false ?

restructuring the code makes it rather unwieldy, or at least I have no idea of how to make a lightweight change here. The whole thing seems more like a limitation of the test system to express such tests.

this line and line 348 above should be removed

isn't it better to explicitly declare variables that are used?

oh jesus, i finally noticed where this variable is used. the better solution would be to declare it in-scope, in lines 403 and 405 below.

403-406 becomes

if (a < b) {
  long t = a;
  a = b;
  b = t;
}
while (b > 0) {
  long t = a % b;
  a = b;
  b = t;
}

there are three software engineering/C++ style ideas in this revision:

• declare variables only in the scope where they're used
• don't use a temp variable for two separate purposes
• give each statement its own line

i get that there is a certain appeal to not modifying code beyond the minimal editing needed. there is a discussion to be had about that. what i feel is that since we've pretty much all as a team acknowledged that this code is (1) extremely old and (2) not clearly written, we should take some extra effort to clean it up a bit while we're in there. i have gone overboard in some cases ( #2861 is an example of that) but if it's a matter of writing

if (a == 0) return b;

instead of

if (a == 0)
  return b;

when the first is what was done 20 years ago but the second is clearly better C++ style, i have to confess i don't see the point in sticking with the former. these are the kinds of things that i don't think we need a style guide to agree on, but maybe that is a faulty assumption since you and other contributors are not frequently working in C++. if so i am happy to go through that more transparent process.

yes, just let me know, I'll try and adhere to this. Thanks!

we don't need this kind of redundancy in tests. we just need to test the simplest instance and the continuation. in this case, the simplest instances are all combinations of signs (the six pairs you can make from {-1, 0, 1}) and the continuation is the same thing including {-2, 2}. anything more than that is wasted effort and only serves to obfuscate the purpose of the test. in this case the set (-4..4).dup(2).allTuples is convenient notation but provides much more than what is needed in both range and combinatorics. it will test the commutative property twice on many combinations ([3, 0], [0, 3]). same with line 188 below, 729 assertions where just one would do, and in line 202, 9 assertions when 5 would do.

Knowing the implementation, I agree.

I had assumed that the implementation should be unknown from the perspective of the test, so that also other numbers may fail.

(changed this)