Re: [lmi] PATCH: use std::uncaught_exceptions()

[Vadim Zeitlin]

On Thu, 29 Mar 2018 20:18:56 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2018-03-25 16:40, Vadim Zeitlin wrote:
[...]
GC> >  Alternatively, and more verbosely, but also less magically, we could just
GC> > stop calling wxPdfDC::EndDoc() from pdf_writer_wx dtor automatically and
GC> > require the code using this class to call some close() method explicitly.
GC> 
GC> Yes, let's do that, please.

 Done now, please see https://github.com/vadz/lmi/pull/78, but after
implementing this, I like it significantly less than before because of all
the additional checks I felt compelled to add. Basically, we lose the
compile-time invariant ensuring that the dtor of the object is called once
and once only and, importantly, that the object can't be used after being
destroyed and replace it with a run-time invariant that save() must be
called once and once only and that no other methods can be called after it.
And this just doesn't seem like a good trade-off.

 As an aside, it's really unfortunate that there is no way to let the
compiler check that the object is not used after being moved. Apparently
there was a "destructive move" proposal (N4034), but it seems to have been
shelved back in 2015 and is not planned to be added in C++2a. Clang-tidy is
supposed to be able to detect this problem
(https://clang.llvm.org/extra/clang-tidy/checks/bugprone-use-after-move.html),
but Debian version doesn't seem to have any checks at all, so I've been
unable to test this.


GC> > I suspect that you prefer this solution, but I'd still like to check for
GC> > uncaught_exceptions() in wxPdfDC dtor to assert (or warn?) if close()
GC> > hadn't been called without an active exception, as this would be a logic
GC> > error in this case.
GC> 
GC> The perceived advantage of implementing the final phase of the task
GC> in a dtor is that the dtor is certain to be called at the right
GC> moment.

 As mentioned above, there is more than this. I.e. this is advantage #1,
but the advantage #2 is that it's also certain to be called only once,
while nothing prevents calling save() twice (at compile-time) and the
advantage #3 is that no other members can be called on the destroyed
object, which is again not true in the solution with save().

GC> But please review branch 'odd/dtor-verifies-postcondition', where
GC> I believe I've done what you recommend above, for the progress_meter
GC> class. The dtor notifies us whenever we've followed an execution
GC> path that doesn't call culminate() as it should (which would be a
GC> logic error).

 Yes, my code is very similar to this branch, but it does have more checks
because I think it's better to add all the boilerplate rather than risk
wasting time debugging weird bugs in the future due to not respecting the
invariants these checks verify.


GC> >  To summarize, my preferred solution would be to continue calling EndDoc()
GC> > from the dtor automatically, but skip doing it during stack unwinding. But
GC> > if you prefer the other one, as I suspect, I could live with it as well.
GC> 
GC> Yes, I do strongly "prefer the other one" as you suspected.

 I don't really expect you to change your opinion even in spite of the
arguments above, but to me it now seems clear that saving the PDF document
from pdf_writer_wx dtor is a better solution. Just compare the changes at
the URL above with just adding a single uncaught_exceptions() check to
pdf_writer_wx dtor, i.e. writing it like this:

---------------------------------- >8 --------------------------------------
pdf_writer_wx::~pdf_writer_wx()
{
    // Don't save the file if we're unwinding the stack due to an
    // active exception.
    if(!std::uncaught_exceptions())
        {
        pdf_dc_.EndDoc();
        }
}
---------------------------------- >8 --------------------------------------


GC> Consider what happens here if EndDoc() throws:
GC> 
GC> pdf_writer_wx::~pdf_writer_wx()
GC> {
GC>     // This will finally generate the PDF file.
GC>     pdf_dc_.EndDoc();
GC> }

 This is completely unrelated to the current discussion, but there is
actually a big problem here, which is that EndDoc() doesn't throw (because
wxPdfDocument library doesn't use exceptions at all) and, worse, it doesn't
even return anything to allow us to check for its success. So we actually
should use GetPdfDocument()->SaveAsFile() here, which does return bool, but
first I need to submit a patch to wxPdfDocument to fix half-missing error
checking in SaveAsFile() too...

GC> Can we "fix" it by placing it in a try-block?

 It would be wrong to "fix" it in this way. If saving the document could
throw, the exception would need to be propagated because, quite clearly, we
can't meaningfully handle it in pdf_writer_wx dtor itself. So the correct
fix would be to make this dtor noexcept(false).

GC> OTOH, if we write "pdf_dc_.EndDoc();" in a close() function,
GC> we don't have to ask ourselves these questions.
[...]
GC> I think that (as in branch 'odd/dtor-verifies-postcondition')
GC> we should instead write:
GC> 
GC>     if(extra_pages_ && !std::uncaught_exceptions())
GC>         {
GC>         safely_show_message("Page count is wrong: please report this.");

 Yes, this is indeed a good idea, thanks.

GC> IOW, we're skating on thin ice when we code a dtor, and any
GC> misstep may be catastrophic. Writing the body of a dtor calls
GC> for almost as much caution as coding a C signal handler. In
GC> a way it's much worse, as seen with the deliberately-throwing
GC> dtor that I tried to write but couldn't: every dtor all the
GC> way up the inheritance chain must be marked "noexcept(false)",

 AFAICS this is incorrect. Only the base class dtor needs to be marked in
this way.

GC> but if we miss marking a single one, then the ice breaks and we
GC> fall through...and gcc won't warn us. It feels like we're
GC> fighting the language, and the compiler isn't on our side.

 Yes, and this is regrettable. But it's not as bad as described, and this
is not always relevant: for example, pdf_writer_wx doesn't have any base
classes in the first place, so I don't see any real problem with making its
dtor noexcept(false).


 Actually, thinking back to the beginning of this discussion, I think that
the matters could be confused by simultaneously discussing two quite
different issues here. The first of them, which started this discussion,
was throwing an exception if a page number mismatch was detected in
numbered_page, instead of just giving a warning() in this class dtor. This
issue is still _not_ addressed by the PR above and I'll make a separate one
for it, using more or less the same approach and just introducing a
post_render() method for performing such checks, instead of doing them in
dtor, which I think would be the right thing to do here, as all page
classes are used only by pdf_illustration itself and so it's not a problem
to impose that their pre_render(), render() and, in the near future,
post_render(), methods are called in order.

 However then we've somehow decided to also do the same thing for
pdf_writer_wx, where the problem is completely different: it's not about
exceptions being thrown from the dtor, but rather about EndDoc() being
called from the dtor even during stack unwinding, when it shouldn't be. And
this problem is, IMO, solved in a much simpler and less error-prone way by
just adding std::uncaught_exceptions() check as shown above, than by doing
everything I did in the PR 78.

 Having said all this, I now wonder if I was over-pessimistic in my
expectations expressed above and if you could, after all, be persuaded to
change your mind and to use the simpler approach for pdf_writer_wx dtor?
Once again, this doesn't change the fact that we need to change the ledger
PDF generation code to throw exceptions (and not from the dtor) in case of
the page number mismatch.

 Please let me know what do you think, thanks,
VZ