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

[Vadim Zeitlin]

On Mon, 2 Apr 2018 00:52:46 +0000 Greg Chicares <address@hidden> wrote:

GC> As I see it, "use dtors only for releasing resources" is a best practice,
GC> and contravening a best practice for convenience is an unfavorable tradeoff.

 I disagree. RAII is just a particular, albeit very useful, special case of
using dtors to guarantee performing some action on scope exit. C++ dtors are
more general purpose than this and this is why C++ doesn't have try/finally
as some other languages do (or, conversely, why C# has "using" or Python
has "with" statements which are not needed in C++).


GC> Why worry about calling save() twice in this particular case,

 Calling EndDoc() a different number of types from StartDoc() seems wrong
to me, and I'm not sure what is it going to do. In the best case, this will
write the file twice, which is totally unnecessary.
 
GC> when we have similar functions that we don't worry about? For example:
GC>   void database_impl::save(std::ostream& index_os, std::ostream& data_os)
GC>   void single_cell_document::write(std::ostream& os) const

 This is different: it is perfectly safe and, indeed, possibly useful, to
call save() with 2 different paths to save the same database to 2 different
files, for example. With pdf_writer_wx::save(), this is not the case, as
the filename is specified in the ctor and can't be changed later. I
hesitated for some time between calling the method save() or close() and it
seems like I chose the wrong name, finally, because the above shows that
it's really different from the database class method with the same name. So
we probably should rename it to close(), or maybe save_and_close(), to
emphasize that the object can't be used any more after calling it.

GC> BTW, AFAICT, wxPdfDocument::EndDoc() doesn't just "end" a document--it might
GC> even write the whole thing, starting with the "%PDF-1.3" in the first few
GC> bytes.

 Yes.

GC> If that's the case, then this:
GC>   ~pdf_writer_wx() {if(!std::uncaught_exception()) pdf_dc_.EndDoc();}
GC> would actually write the file, and otherwise it wouldn't be written at all.

 Yes, this is why we do it like this.


GC> I completely agree. It's much like using a free()d C pointer. C++17
GC> [20.5.5.15] says "moved-from objects shall be placed in a valid but
GC> unspecified state": IOW, they're "undead", and I've watched enough
GC> horror movies to develop an abhorrence for the undead.

 The rationale is, IIUC, that you should be able to assign to a moved-from
object and reuse it for something else. In theory, this could be slightly
more efficient than creating a new object, but IMO allowing it at the
expense of safety is a very dubious trade-off. But well, it is the way it
is.


GC> There's something else I don't understand: why use move semantics here?

 Solely to make it clear in the code calling save() that it's the last
thing that can be done on the object and to prevent it from being called
twice accidentally. The idea is that if you see

        std::move(pdf).save();
        pdf.output_xxx(...);

in the code, you would immediately realize that something is wrong here
because the object is not supposed to be used after being moved from (and,
hopefully, in the future clang-tidy might detect it automatically).

GC> Is there some object that is actually moved?

 No. It's just a way to indicate that the object is "consumed" by the call
to save() and can't be used afterwards.

GC> Or did you ref-qualify save() only to prevent any other member function
GC> (other than the dtor) from being called after save()?

 Yes.

GC> If so...is that goal really so important,

 I don't know how to quantify "so", but I think it's better to do it, than
not (of course, I also think that it's better to not have save() in the
first place and write the file from the dtor in the first place).

GC> and is there no other way to achieve it?

 I don't know of any.

GC> > GC> But please review branch 'odd/dtor-verifies-postcondition', where
GC> > GC> I believe I've done what you recommend above, for the progress_meter
GC> > GC> class. The dtor notifies us whenever we've followed an execution
GC> > GC> path that doesn't call culminate() as it should (which would be a
GC> > GC> logic error).
GC> > 
GC> >  Yes, my code is very similar to this branch, but it does have more checks
GC> > because I think it's better to add all the boilerplate rather than risk
GC> > wasting time debugging weird bugs in the future due to not respecting the
GC> > invariants these checks verify.
GC> 
GC> Are there other checks that I should add to progress_meter?

 You don't need to do anything in reflect_progress() as there is already a
check for "max_count_ <= count_" (it's another losing battle, but I have
real trouble reading this condition because we're not checking max_count_
here, we really want to compare count_ with the maximum value...), so any
calls to this method after culminate() would already be detected. You might
want to add a similar check to dawdle(), however, as I don't think it makes
sense to call it after culminate().

 I also do think that you should add a check for calling culminate() twice
as this would be a bug resulting in an extra newline being printed out when
using the CLI version of the class. Of course, it's not a big deal -- but
it's still a bug. And, just as with pdf_writer_wx, it would be so much
simpler to get rid of culminate() and just do it in the dtor, wouldn't it?


GC> It comes down to whether or not we believe that dtors should be used
GC> only for releasing resources.

 Yes, and I don't believe that. Of course, we can impose such a limitation,
but it's self-defeating as we then end up emulating dtors ourselves.

GC> Instead, we could regard a dtor as a combination of two functions:
GC> 
GC>   C::~C() noexcept(false)
GC>   {
GC>     if(!std::uncaught_exceptions())
GC>       { finalize(); }
GC>     destroy();
GC>   }
GC> 
GC> Then the usual advice
GC>  - never allow an exception to escape from a dtor
GC> might arguably be refined:
GC>  - never allow an exception to escape from destroy()
GC>  - but feel free to throw in finalize() [*arguably*]

 This is a good summary, yes.

GC> Of course, that's not quite right: the Committee had a reason for
GC> replacing
GC>   bool std::uncaught_exception() // deprecated by C++17
GC> with
GC>   int std::uncaught_exceptions() // C++17
GC> but the code above in effect uses the deprecated function. Yet it
GC> was generally thought that the deprecated function wasn't useful:
GC> 
GC> http://www.gotw.ca/gotw/047.htm
GC> | My major problem with this solution is not technical, but moral:
GC> | It is poor design to give T::~T() two different semantics ...
GC> | I do not know of any good and safe use for std::uncaught_exception.
GC> | My advice: Don't use it.

 I wouldn't say "generally". Alexandrescu obviously did think it was useful
and there are plenty of other libraries built entirely on performing
(possibly throwing) operations in dtors of temporary objects.

GC> Now, of course, I do understand that we could
GC> 
GC>   C::C() : exception_count_(std::uncaught_exceptions())
GC>   {int const exception_count_;}
GC> 
GC>   C::~C() noexcept(false)
GC>   {
GC>     if(exception_count_ != std::uncaught_exceptions())
GC>       { finalize(); }
GC>     destroy();
GC>   }

 Yes, exactly. And while this is not very important for pdf_writer_wx, it
would be arguably worth doing for progress_meter because it might
conceivably be used from a dtor (showing the progress of closing
something?). And it's definitely crucial for Boost.Log library mentioned as
example at http://en.cppreference.com/w/cpp/error/uncaught_exception

GC> (and I agree with your reasoning that it
GC> 
GC>     // seems highly unlikely that a pdf_writer_wx object would ever be 
created
GC>     // in a dtor of some other object, which is the only situation in which 
the
GC>     // two versions would behave differently
GC> 
GC> in our concrete case), but--in the general case--how does that
GC> address the two objections?
GC> 
GC> - It resolves the technical objection. It makes ScopeGuard work.
GC> (I'm just assuming that, arguendo; I haven't studied it enough to be
GC> certain myself.)

 Yes, the assumption is correct. ScopeGuard can now be made to work
reliably in C++17.

GC> BTW, as a corollary:
GC>   ~pdf_writer_wx() {if(!std::uncaught_exception()) pdf_dc_.EndDoc();}
GC> would have been as good a solution in C++98 days as it is now,

 Yes.

GC> even though everyone was against it then.

 No, again, I don't think "everyone" is correct. I've been using a library
performing database operations in dtor (and throwing exceptions from them)
called SOCI (http://soci.sourceforge.net/) for many years without any
problems related to throwing from dtor and I'm definitely glad to have been
able to use its nice syntax during all this time instead of having to call
execute_query(), as with many more traditional database access libraries,
manually for all the statements.

GC> - It ignores the moral objection. C++ chooses not to provide a
GC> "finally" construct:
GC>   http://www.stroustrup.com/bs_faq2.html#finally
GC> Now std::uncaught_exceptions() lets us emulate it, more or less, in
GC> a byzantine way, and it might behave as we hope if we're extremely
GC> careful.

 Sorry, I think it's an exaggeration. There is nothing byzantine about it,
dtors are, by design, supposed to be used for the same purpose as
try/finally in other languages.

GC> But when I tried, I got std::terminate(). I thought that was
GC> because I didn't write "noexcept(false)" in all the right places,
GC> but I guess I was wrong, and I just don't understand it at all.

 But you do understand it and the problem was indeed that the base class
dtor hadn't been noexcept(false). The problem is just that gcc didn't
correctly diagnose the error of using noexcept(false) with "= default",
which basically meant that noexcept(false) was ignored. We've seen that
clang gave a much better error for this and we can hope that future
versions of g++ might do better here as well. FWIW I've opened a bug
report for this at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85155


GC> Or maybe I'm conflating the idea of doing actual processing in a dtor
GC> with "finally". If C++ had such a feature, would you write:
GC>   try {write a PDF page}
GC>   catch {handle some possible exception}
GC>   finally {pdf_dc_.EndDoc();}
GC> ?

 No, because "finally" is not flexible enough. It only allows to implement
the traditional (as described in the original paper) "on fail" ScopeGuard,
but not the "on success" ScopeGuard from the improved and revised later
versions of the same idea that we need here.

GC> And maybe I'm confusing your suggestion with Alexandrescu's
GC> ScopeGuard, which AFAICT is the reason why std::uncaught_exceptions()
GC> was added in C++17; but you wouldn't want to use ScopeGuard here,
GC> would you?

 I would use the "scope(success)" variant of the ScopeGuard, using the D
language terminology, which is a refinement of the original "scope(exit)"
idea, corresponding to "finally".

 In fact, my suggestion for saving the file in pdf_writer_wx dtor is
equivalent to using a "on success" ScopeGuard in exactly the same way as
using RAII is similar to releasing resources in a classic "on exit"
ScopeGuard: i.e. instead of doing the check in an external scope guard,
it's done inside the class itself.

 Or, to reformulate it once again, my proposal is exactly equivalent to

        void some_function()
        {
            pdf_writer_wx writer{...};
            SCOPE_SUCCESS({ writer.save(); });
            ...
        }

Would you find this as offensive as calling save() from dtor?


GC> >  Having said all this, I now wonder if I was over-pessimistic in my
GC> > expectations expressed above and if you could, after all, be persuaded to
GC> > change your mind and to use the simpler approach for pdf_writer_wx dtor?
GC> 
GC> No, the more I think about this, the more I dislike that idea.
GC> C++ dtors are intended for cleanup.

 As, I think, I made abundantly clear above, I disagree. They're very
useful for cleanup, but it doesn't mean they shouldn't be used for anything
else. We have all the nice, compiler-generated machinery, ensuring that
dtors of local (and member) objects are called once and only once and after
any other object method calls -- why not use it?

GC> ScopeGuard stretches that a bit to use them for rollback: a niche use
GC> case. Using a dtor in class progress_meter to log a message if an
GC> invariant has not been fulfilled is another inoffensive extension. But
GC> using a dtor to perform work required to establish a postcondition
GC> seems wrong.

 Why? It looks like a very natural place to me.

GC> We've observed that well-formed but incorrect PDF files are created
GC> when the original dtor was called. Maybe we could patch that up by
GC> bifurcating the ctor into finally() and destroy() subparts as above;
GC> I'm not convinced that makes everything kosher, and I'm still afraid
GC> of what happens if an exception is thrown. But I suspect that problem
GC> may not have occurred if we had spelled out the procedural steps:
GC>   open file
GC>   write cover page
GC>   write first section
GC>   write another section
GC>   close
GC> instead of using a dtor for mainstream processing.

 Well, yes, this is what the PR that you applied did. But it's not nearly
as elegant because it requires all the boilerplate code for checking that
we didn't forget to call close(), that "write something" is not called
after close() had been called etc.

 It's not a question of whether we can write code like this: of course we
can, after all we could just use pure C subset of C++ which doesn't include
dtors at all, and we could still write anything we wanted in it. It's
rather the question of what is the simplest and least error-prone way of
doing it and for me the comparison is clearly one-sided in favour of using
dtors.

 It would, IMHO, be a mistake to assign too much importance to your bad
experience with numbered_page dtor: first, a much more complicated case
than that of pdf_writer_wx or progress_meter because this class was a
member of a polymorphic class hierarchy and second, this experience was so
bad only because of the bad gcc error message.

 Regards,
VZ