Re: [lmi] [lmi-commits] master 38df10a 4/9: Simplify diagnostics

[Vadim Zeitlin]

On Wed,  4 Apr 2018 21:27:37 -0400 (EDT) Greg Chicares <address@hidden> wrote:

GC> branch: master
GC> commit 38df10aea7d2a007d2dae4c9c1c880ab60cd4a0d
GC> Author: Gregory W. Chicares <address@hidden>
GC> Commit: Gregory W. Chicares <address@hidden>
GC> 
GC>     Simplify diagnostics
GC>     
GC>     The argument for simpler diagnostics in the preceding commit applies
GC>     here, but there is a stronger reason in this case: construction of the
GC>     replaced diagnostic message could throw, causing an exception to escape
GC>     from a destructor.
GC> ---
GC>  pdf_writer_wx.cpp | 9 +--------
GC>  1 file changed, 1 insertion(+), 8 deletions(-)
GC> 
GC> diff --git a/pdf_writer_wx.cpp b/pdf_writer_wx.cpp
GC> index e25408f..ffff225 100644
GC> --- a/pdf_writer_wx.cpp
GC> +++ b/pdf_writer_wx.cpp
GC> @@ -34,7 +34,6 @@
GC>  
GC>  #include <exception>                    // uncaught_exceptions()
GC>  #include <limits>
GC> -#include <sstream>
GC>  
GC>  namespace
GC>  {
GC> @@ -272,12 +271,6 @@ pdf_writer_wx::~pdf_writer_wx()
GC>      // would just result in incorrectly skipping the check, i.e. not 
critical.
GC>      if(!std::uncaught_exceptions() && !save_has_been_called_)
GC>          {
GC> -        std::ostringstream oss;
GC> -        oss
GC> -            << "Please report this: PDF file \""
GC> -            << print_data_.GetFilename().ToStdString(wxConvUTF8)
GC> -            << "\" was not saved."
GC> -            ;
GC> -        safely_show_message(oss.str());
GC> +        safely_show_message("Please report this: save() not called for 
PDF.");
GC>          }
GC>  }

 But the argument for making the error message useful from my previous
reply applies even more strongly here too: this message is supposed to be
seen by the end users, and we really, really need to tell them which file
we failed to produce. Don't you just hate the error messages that tell you
that "something went wrong" without telling you what exactly happened? This
would be a prime example of things not to do in my book...

 And, in fact, the argument of ostringstream ctor throwing is much weaker
than it would seem. The only possible exception that it could throw would
be std::bad_alloc, but if we're so short on memory that we can't allocate
enough of it for this (short) string, then we certainly are going to crash
very soon anyhow, even if lmi is perfectly written to handle all memory
allocation failures (which is so difficult that I'm really not sure if this
is the case) because wxWidgets definitely isn't and will crash if
allocating (small amounts of) memory fails. So, in practice, we're never
going to throw here because if memory so tight, the program would have
already crashed anyhow.

 But if you really want to guard against this, for some reason, it would be
still better to a put a try/catch around the code using std::ostringstream
and give an actually useful error message.

VZ