[lmi] "Alert" messages [Was: Code review: product editor]

[Greg Chicares]

On 2007-3-2 10:59 UTC, Evgeniy Tarassov wrote:
> 
> IIRC it is not advised to add return after a call to fatal_error().
> Does it apply to warning() too?

The simple answer to the question posed is:
 - don't routinely add anything after either of those
 - but add a return if it would be obviously wrong not to
 - and add an explicit throw-expression only if gcc wouldn't
     compile without it

The simple answer to an underlying question not posed is:
 - rarely write warning(): fatal_error() is usually better

In more detail...

> IIRC it is not advised to add return after a call to fatal_error().

I prize consistent guidelines that let us write in one uniform
style without thinking [0], so thanks for raising the question
and pointing out some of the inconsistencies (they are legion,
I know, but we're gradually working on them). Rearranging your
email slightly:

> global_settings.cpp
>        fatal_error() << "Instantiation failed." << LMI_FLUSH;
>        throw std::logic_error("Unreachable"); // Silence compiler warning.
> ihs_avmly.cpp
>        fatal_error() << [...] << LMI_FLUSH;
>        return;

In the first snippet, a simple 'return;' wouldn't work because
it's not a void function. If it returned, say, a double, then
we could write 'return 0.0;'; I've probably done that elsewhere.
Here, however, a reference must be returned, and that's harder,
because we'd need an object to bind the reference to. We can
avoid that problem by using a throw-expression instead. That's
better, because we can always throw.

Now, what (uniform) throw-expression would be best? Consider:

  throw;
That's always well defined [15.1/8], but doesn't do what we
require.

  throw std::logic_error("Unreachable"); // Silence compiler warning.
As quoted above: pretty good, but requires <stdexcept>.

  throw lmi_unreachable_exception_to_silence_compiler_diagnostic();
We could write a function like this in 'alert.hpp', and
document it there. (I wouldn't write a macro instead,
because this doesn't require a macro.)

  throw "Unreachable--silences a compiler diagnostic.");
Self documenting, and no <stdexcept> dependency. I think I
like this best; have you any other suggestions?

Now, where should such an expression be written?

 - after every 'fatal_error() << [...] << LMI_FLUSH;'?
No, that's often unnecessary, and creates busywork while
detracting from comprehensibility.

 - whenever we want to silence a compiler diagnostic
   following 'fatal_error() << [...] << LMI_FLUSH;' or
   any other expression that necessarily throws
Corollary: routinely write nothing after fatal_error();
this guideline comes into effect only when gcc complains.
That sounds like the right way to me--do you think otherwise?

> IIRC it is not advised to add return after a call to fatal_error().
> Does it apply to warning() too?

I've come to think that warning() is rarely appropriate,
and that it was a mistake for me to use it so widely in the
past--for somewhat the same reason that C++ exceptions were
designed with termination semantics: actual practice showed
that resumption semantics are rarely useful when an error
occurs.

In general, instead of
  warning() << "..." << LMI_FLUSH; return ...;
I'd try to write
  fatal_error() << "..." << LMI_FLUSH;
Given how lmi and wx handle C++ exceptions, this normally
does the right thing; I can't think of any case where I've
seen it not work, other than program-initialization errors
that arise before the wxLog classes have started up (but
there's a wx solution for that situation already, and we
probably don't have to discuss that further here).

Yet there are some special cases where warning() is useful,
either with or without a following 'return'. Examples:

  warning() << "Unable to read file '" << filename << "'." << LMI_FLUSH;
  return false;
Here, an operation has failed, and there's no good way to
overcome that. This occurs in an OnCreate() override, and wx
expects the 'false' return so that it knows how to proceed
correctly--and I guess throwing an exception would prevent
that from working. Thus, wx overrides that return a status
code are a special case. (I don't think lmi has many other
functions that pass error codes upstream.) This looks like
resumption semantics, but it resumes only in order to
propagate an error code that triggers termination semantics
upstream for the requested action.

  warning() << "Solution not found: using zero instead." << LMI_FLUSH;
Here, the user tried something that would usually work, but
didn't in the particular circustances. An illustration is
produced nevertheless, and it's not invalid, but it's not what
the user hoped for. No early exit is wanted in this case, so
you wouldn't write 'return;' or use fatal_error(). This is one
of the rare examples that really does call for an intrusive
notification without resumption semantics.

In 'view_ex.cpp' I see two interesting examples [reformatted]:
  wxIcon icon = wxXmlResource::Get()->LoadIcon(z);
  if(!icon.Ok())
    {warning() << "[...] invalid icon; using default." << LMI_FLUSH;}
  return icon;
This one's okay: wx supplies a default icon, and the program
can resume; but there's probably a problem with the way the
program is installed, and we'd like the user to know that and
tell us. However, the next example is not okay:
  wxMenuBar* menubar = wxXmlResource::Get()->LoadMenuBar(z);
  if(!menubar)
    {warning() << "[...] null menubar pointer." << LMI_FLUSH;}
  return menubar;
This exhibits resume-then-crash semantics. I need to fix it;
compare IllustrationView::MenuBar() to DatabaseView::MenuBar()
and you'll see why. It's unreasonable to ask the author of a
new ViewEx class to trap the error: DatabaseView::MenuBar()
can crash, while IllustrationView::MenuBar() cannot, but it's
ViewEx::MenuBarFromXmlResource()'s fault because it should
never have returned that null pointer. I dread null pointers.

Sometimes I use warning() for debugging statements, often
aligned with the left margin and commented out. Those don't
really belong in production code; some have made their way
into HEAD, but that's a mistake we should avoid in new code.

> Comments from alert.hpp:
> [...]
> /// fatal_error: Dire runtime problems that prevent the system from
> /// performing a requested action running in any reasonable manner.
                                    ^^^^^^^ oops, extraneous word
> /// Generally, an exception would be thrown; a gui might catch it,
> [...]
> /// warning: Significant runtime problems that should be brought to the
> /// user's attention: the program may work, but not in exactly the way
> /// the user wanted. A gui would probably use a messagebox here.
> 
> "Generally" (for fatal_error()) and "would probably" (for warning())
> and exception will be thrown.

I will clarify that as shown in a footnote [1]. My intention
was to specify a very general, abstract API, with hints about
what C99 3.16 would call "recommended practice". Maybe I spend
too much time reading ISO standards.

Just to be very clear: for warning(), it is not "recommended",
and never was, that an exception be thrown; now [1] it's
explicitly forbidden.

> Some existing code snippets from lmi:

[snip fatal_error() examples]
I'd like to rewrite them all this way (see above):
  throw "Unreachable--silences a compiler diagnostic.");

> Would it be correct to say that it is advised to throw a logical error
> after a fatal_error(),

I think we should throw explicitly only when a good compiler
complains with stringent warnings enabled.
 - gcc:    I *enforce* clean compiles
 - comeau: I *strive* for clean compiles
(There are a few other good compilers, but I don't have them.)
 - borland: I've given up hope--too flaky

> and a warning() should be followed by a return
> statement?

Actually, I think we shouldn't automatically or even often
write a return statement after a warning(). As discussed
above, functions that return a status code are a special
case, though, which probably arises frequently in wx
overrides but rarely elsewhere in lmi.

But in general I think fatal_error() will do just fine for
almost everything in the product editor. The only place I
can think of (off the top of my head) where warning() seems
really desirable there is where an entity with more than
one million elements would be created, and I think you
handle that with an appropriate wx mechanism already.

> In both cases the reason to add something is that in the
> future there could be an implementation which does not throw.

Well, warning() doesn't throw, and a future implementation
that throws would be wrong, so I'll make the documentation
say that [1], and also specify that an exception must be
thrown when the ostream returned by fatal_error() is flushed.

> The
> difference is due to fatal_error() being much less likely to ever let
> program flow to return from it, right?

It'd be wrong for fatal_error() not to throw [1] when the
ostream it returns is flushed, or for warning() to throw.

The really bizarre one is hobsons_choice(), which makes the
user decide whether to throw. I'm trying to do away with it.
It feels like INTERCAL. The "reason" for it is that I used
to add assertions without enough caution, and failed to make
the time to fix the underlying problems they identified, so
I needed to offer users a way to bypass assertion failures.
That's the road to perdition, let me assure you. Slowly we
are clawing our way back.

---------

[0] "without thinking":

  It is a profoundly erroneous truism repeated by all
  copybooks, and by eminent people when they are making
  speeches, that we should cultivate the habit of thinking
  of what we are doing. The precise opposite is the case.
  Civilization advances by extending the number of
  operations which we can perform without thinking about
  them. Operations of thought are like cavalry charges in
  battle--they are strictly limited in number, they require
  fresh horses, and must only be made at decisive moments.

  --Alfred North Whitehead, "Introduction to Mathematics"

[1] Revised 'alert.hpp' documentation that I plan to commit
(comments are invited):

/// Here's how the various streams are intended to be used. These
/// descriptions are generic; the implementation for each interface
/// documents specific choices made for 'lmi'.
[...]
/// warning: Significant runtime problems that should be brought to
/// the user's attention: the program can continue, but not in exactly
/// the way the user wanted, so resumption semantics are appropriate.
/// All interfaces must display a message but not throw an exception;
/// a GUI would typically use a messagebox, for instance.
[...]
/// fatal_error: Dire runtime problems that prevent the system from
/// performing a requested action running in any reasonable manner,
/// and therefore call for resumption semantics. All interfaces must
/// show a message and throw an exception when the stream is flushed.
/// A GUI might catch that exception, terminate the action in an
/// orderly way, and yet remain active, while a command-line program
/// might simply terminate.
[...]
/// Usage notes.
[see the existing first paragraph, which is unchanged]
///
/// Sometimes gcc warns of a missing return statement after the stream
/// returned by fatal_error() is flushed: an exception is obligatorily
/// thrown, but it is difficult for a compiler to discern that. When
/// gcc issues such a diagnostic, add exactly this line:
///   throw "Unreachable--silences a compiler diagnostic.");
/// to silence it. Rationale: a return statement would work, but might
/// not be trivial to write (the function may return a reference) and
/// could not be everywhere identical. The prescribed throw-expression
/// always works, is self documenting, and adds no header dependency.
/// Writing it the same way everywhere requires no invention and
/// increases global comprehensibility.