Re: [lmi] [lmi-commits] master ecd1362 3/9: Shorten diagnostics

[Vadim Zeitlin]

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

GC> branch: master
GC> commit ecd1362df9f779cd9684f14aea6e40546b9bb351
GC> Author: Gregory W. Chicares <address@hidden>
GC> Commit: Gregory W. Chicares <address@hidden>
GC> 
GC>     Shorten diagnostics
GC>     
GC>     The benefit of elaborate assertions is that they specifically indicate
GC>     what has gone wrong. The cost is that they make the code longer and
GC>     harder to read, and that they must be maintained if the variables and
GC>     functions they use ever change. It's a tradeoff, and a matter of taste,
GC>     but in this case simplicity seems preferable.
[...keeping just one of the changes for reference...]
GC>  wxDC& pdf_writer_wx::dc()
GC>  {
GC> -    LMI_ASSERT_WITH_MSG
GC> -        (!save_has_been_called_
GC> -        ,"Can't use device context of the PDF file \""
GC> -            << print_data_.GetFilename().ToStdString(wxConvUTF8)
GC> -            << "\" which was already saved"
GC> -        );
GC> -
GC> +    LMI_ASSERT(!save_has_been_called_);
GC>      return pdf_dc_;
GC>  }

 Sorry, but I just have to disagree. This change makes the assertion
failure completely useless for diagnosing the problem as the information
about which method exactly was called when it happened is now lost. You may
consider that giving the filename in the message is redundant, because the
user already knows which PDF file was being generated at the moment when
the assertion was generated, but even then I'd still keep it because it's
far simpler to tell people to copy the assertion message rather than
telling them to copy the assertion message and also think about what were
they doing when it happened.

 But making all 3 assertions identical is just completely
counter-productive. It doesn't cost anything to use a unique message for
each of them, there is no tradeoff whatsoever here because there is no
maintenance to speak of, while the benefit of knowing where exactly the
problem occurred is clear.

 Maybe the need for specifying the message could be avoided by using
__FUNCTION__ inside LMI_ASSERT(), but as long as we don't have it there
I really think this change should be reconsidered.

 Regards,
VZ