[lmi] DTSTTCPW, concinnity, and...are we done yet?

[Greg Chicares]

Vadim reported a problem with this line
    return fegetround(rounding_mode);
in 'fenv_lmi.cpp'. I'd like to discuss it in light of "agile"
practices such as those advocated here:
  http://extremeprogramming.org/
especially DTSTTCPW.

On line 98 here:
  http://cvs.sv.gnu.org/viewcvs/lmi/lmi/fenv_lmi.cpp?annotate=1.15
the problem is obvious. 'rounding_mode' is a cut-and-pasto, and
the C99 function fegetround() takes no argument. Taking one step
back and looking at the next conditional block, we see a candidate
for more appropriate copying and pasting. Maybe the simplest thing
that could possibly work is this:

-    return fegetround(rounding_mode);
+    int z = fegetround();
+    return
+          (FE_TONEAREST  == z) ? fe_tonearest
+        : (FE_DOWNWARD   == z) ? fe_downward
+        : (FE_UPWARD     == z) ? fe_upward
+        : (FE_TOWARDZERO == z) ? fe_towardzero
+        : fenv_rounding_error()
+        ;

except that it doesn't compile (under GNU/Linux; msw users can
manipulate the conditional directives to try compiling it with
MinGW): grep the sources for 'fenv_rounding_error' to see why.

We could clone fenv_rounding_error(), but instead let's recognize
that it's brittle--and, in fact, altogether unnecessary:
  http://groups.google.com/group/comp.lang.c++.moderated/msg/7d4b408b7861072d
so let's try

-    return fegetround(rounding_mode);
+    int z = fegetround();
+    return
+          (FE_TONEAREST  == z) ? fe_tonearest
+        : (FE_DOWNWARD   == z) ? fe_downward
+        : (FE_UPWARD     == z) ? fe_upward
+        : (FE_TOWARDZERO == z) ? fe_towardzero
+        : throw std::runtime_error("fegetround() failed.");
+        ;

Wait...copying and pasting--isn't that wrong?
  http://en.wikipedia.org/wiki/Copy_and_paste_programming
Let's answer the deep questions raised by that article later, but
observe for the moment that it would be less good to approach the
narrow problem in isolation and rewrite the offending statement
as, e.g.,

  // Look out for typos:
  if(FE_TO_NEAREST == fegetround()) return fe_to_nearest;
  else if(...

because then the code would look like

  #if COMPILER_X
    if, else if, ...else logic
  #elif COMPILER_Y
    similar logic written in terms of the ?: conditional operator
  ...

and we'd have introduced a gratuitous barrier to comprehension--as
well as some typos (e.g., how many underscore characters in the
C99 rounding-direction macro above?). Literal copying is better
than writing equivalent code in a different way: it makes
refactoring easier later on; and it makes errors like

    (FE_TONEAREST  == z) ? fe_tonearest
  : (FE_DOWNWARD   == z) ? fe_upward   // These two...
  : (FE_UPWARD     == z) ? fe_downward // ...got switched.
  : (FE_TOWARDZERO == z) ? fe_towardzero

or

    (FE_TONEAREST  == z) ? fe_tonearest
  : (FE_DOWNWARD   == z) ? fe_downward
  : (FE_UPWARD     == z) ? fe_upward
  : (fe_towardzero == z) ? FE_TOWARDZERO

less likely. Some may favor if...else if...else, while others may
prefer switch statements for this sort of work; in this case, I
like the vertically-aligned series of ternary operators, because
mistakes like those simulated above stand out. But above all we
want concinnity, so let's write similar statements similarly. If
we find a different way that really is better, let's improve other
code by rewriting it in that better way.

So...are we done?

No: XP says we need a unit test. Well, we've got one already, in
'fenv_lmi_test.cpp', so we'd better run it.

  Running fenv_lmi_test:
  Expect an (induced) warning now, but no test failure.

  The floating-point control word was unexpectedly '7f'.
  Probably some other program changed this crucial setting.
  It has been reset correctly. Rerun any illustration that
  was being run when this message appeared, because it may
  be incorrect.

  [End of induced warning].

  .... 30 tests succeeded
  no errors detected

The result is exactly as expected. (Of course, we went back and
examined the tests actually performed for the function we changed,
and made sure they provide ample coverage [1].)

Now are we done?

No. Bear in mind that the founding fathers of XP were polemicists
and revolutionaries, and some of their slogans are overweening.
DTSTTCPW could easily be misunderstood as

  Do the quickest thing that could conceivably not be incorrect.

but their intention is very different:

http://c2.com/cgi/wiki?DoTheSimplestThingThatCouldPossiblyWork
|
| DoTheSimplestThingThatCouldPossiblyWork entails two (2) steps:
| first, implement the thing in a simple straightforward way;
| second, refactor the code to produce the simplest system
| including the new thing.

We have good unit tests [1], so we might claim to have made the
smallest change that could possibly work, but that's only the
first of two steps, and must be followed by refactoring.

But nothing in the above patch needs refactoring. Sure, we have
duplicative-seeming code: after all, we copied and pasted
something else. Factoring out what's common there would probably
make the function worse: we don't need a cover function for a
cascade of conditional operators because the language already
provides that natively. And duplication is OK, while triplication
might cross the RefactoringThreshold:

  http://c2.com/cgi/wiki?ThreeStrikesAndYouRefactor
| The ability to distinguish between similar and identical is key
| here. Also, you have to tolerate a certain amount of idiomatic
| duplication.

which is my answer to the overbroad critique of copying and
pasting cited above.

So...are we done yet?

Well, no, not by a long shot. Look:
| refactor the code to produce the simplest system
The goal isn't the simplest change: it's the simplest system. We
found a problem in one place, so we must ask whether similar
problems are likely to exist elsewhere.

  C:/lmi/src/lmi[0]$grep fe.etround *.?pp # First '.' is no typo.
  fenv_lmi.cpp:    fesetround(FE_TONEAREST);
  fenv_lmi.cpp:    int z = fegetround();
  fenv_lmi.cpp:        : throw std::runtime_error("fegetround() failed.");
  fenv_lmi.cpp://   fesetround(precision_mode);
  round_to_test.cpp:    // In case the C++ compiler offers C99 fesetround(), 
assume that
  round_to_test.cpp:    fesetround(mode);

Last time I worked in this area, I did that and decided to ignore
'round_to_test.cpp'. It's a unit test, so if there's a problem
with it, we'll find out soon enough anyway. And the code it tests
should eventually be replaced by C99's round() anyway, so it's not
worth spending a lot of time on. I judge that those reasons remain
valid today, so I'll skip that file.

The other occurrence of /fe.etround/ is in a comment preceding the
implementation of
  void fenv_rounding(e_ieee754_rounding rounding_mode)
It may seem like a good idea to jump in there and make a change
similar to that discussed above. There's even a copy-and-paste
candidate sitting right there. Actually, we did at first rush in
and do that, though it would have been better to read the comment:

  // This C99 function call
  //   fesetround(precision_mode);
  // is nearly equivalent, except that it takes an int argument.
  // It is not conditionally used here, because it seems unwise to
  // weaken the type safety C++ affords merely to implement this
  // one function when its kindred can't be implemented in C99.

Hmmm. Well, comments often lie, and it doesn't seem right to set
the rounding mode from a precision-mode argument; but that's just
a pasto. OTOH, comments often inform, or good ones do, and this
one is giving us a reason not to rush in and make the change that
seems obvious.

Does that mean we should rethink the above patch?

Suppose we discard it and apply this change instead:

- #if defined __STDC_IEC_559__
-     return fegetround(rounding_mode);
- #elif defined __BORLANDC__
+ #if defined __BORLANDC__

That could possibly work, and a simpler change is hard to imagine.
It would leave the system in this simple state:

  #if defined __BORLANDC__
    [compiler-specific implementation]
  #elif defined LMI_X86
      return intel_control_word(x87_control_word()).rc();;
  #else  // Unknown compiler or platform.
  #   error Unknown compiler or platform.
  #endif // Unknown compiler or platform.

The change considered earlier was designed to work with C++
compilers that define the C99 __STDC_IEC_559__ macro, even on
architectures other than 80x86. That may be a real advantage, but
grep easily finds this block

  #if defined LMI_X86
      x87_control_word(default_x87_control_word());
  #else  // Unknown compiler or platform.
  #   error Unknown compiler or platform.
  #endif // Unknown compiler or platform.

in fenv_initialize(), which shows that we'd need to do more work
to secure that advantage. At this point, we've dug in so deep that
we may as well look for high-level documentation in the header
(or maybe we should have done that up front). There we see other
cause for concern:

  [#pragma STDC FENV_ACCESS ON]
  // As of 2005-04-08, the gcc manual here:
  //   http://gcc.gnu.org/onlinedocs/gcc/Floating-point-implementation.html
  // which "corresponds to GCC version 4.1.0" says "This pragma is not
  // implemented".

So we might try to make this architecture-dependent, but C99 says
we get undefined behavior if we don't use
  #pragma STDC FENV_ACCESS ON
in this code, so we'd need a non-80x86 machine with a C99 compiler
other than gcc, which I think none of us has. Furthermore, we see:

  /// Manage the floating-point environment, using C99 7.6 facilities,
  /// where available, in the implementation. Otherwise, use compiler-
  /// and platform-specific techniques where they are known.

That contradicts the comment (presumably written later)

  // It is not conditionally used here, because it seems unwise to

cited above, so we've found a doco that must be fixed. Once we've
done that, we might ask...

Are we done yet?

Well, did we "produce the simplest system"? No. We had identified
fenv_rounding_error() as unnecessary; did we extirpate it? Did we
correct the comment
  //   fesetround(precision_mode);
that we found to be misleading? Should we discard our tested C99
implementation and let someone else reinvent it later when we
could more easily have checked it in (commented out) now?

I'll take care of those things and check revisions into cvs soon.
Once that's finished, then I'd feel entitled to ask:

Are we done yet?

Now, all of this may seem like a lot of work, but it's not really.
It took a while to write it all down, but thinking it through was
only ten minutes' work--and thinking through the design up front
could have saved us work here. It's easy once you make it a habit.
And you don't have to buy into XP's grand world view to see that
some of its practices make a lot of sense.

---------
[1] Vadim points out that the unit tests on lines 175-187 here:
  http://cvs.sv.gnu.org/viewcvs/lmi/lmi/fenv_lmi_test.cpp?annotate=1.5
really aren't as strong as they should be. Others may wish to
figure out what he sees that I didn't, before I shoar them up.