Re: [PATCH 1/5] Rewrite final cleanups

[Lancelot SIX <lsix@lancelotsix.com>]

On Mon, Feb 26, 2024 at 11:53:35AM -0700, Tom Tromey wrote:
> >>>>> "Lancelot" == Lancelot SIX <lsix@lancelotsix.com> writes:
> 
> >> tempdir_name = xstrdup (tempdir_name);
> >> -  make_final_cleanup (do_rmdir, tempdir_name);
> >> +  add_final_cleanup ([] ()
> >> +    {
> >> +      char *zap;
> >> +      int wstat;
> >> +
> >> +      gdb_assert (startswith (tempdir_name, TMP_PREFIX));
> >> +      zap = concat ("rm -rf ", tempdir_name, (char *) NULL);
> >> +      wstat = system (zap);
> >> +      if (wstat == -1 || !WIFEXITED (wstat) || WEXITSTATUS (wstat) != 0)
> >> +	warning (_("Could not remove temporary directory %s"), tempdir_name);
> >> +      XDELETEVEC (zap);
> 
> Lancelot> I am aware that this is orthogonal to this patch and can be address by
> Lancelot> another patch, but in the way to c++ification, this could be replaced
> Lancelot> with:
> 
> Lancelot>   std::filesystem::remove_all (tempdir_name);
> 
> Yeah, I think it's better to do this kind of thing as a separate
> cleanup.
> 
> Also I wonder if all the compilers we support ship std::filesystem.
> (I have no idea.)
> 
> >> void
> >> do_final_cleanups ()
> >> {
> >> -  do_my_cleanups (&final_cleanup_chain, SENTINEL_CLEANUP);
> >> +  for (auto &func : all_cleanups)
> >> +    func ();
> >> +  all_cleanups.clear ();
> 
> Lancelot> I am wondering if we want special handling if one of the cleanup
> Lancelot> function ever throws.  It is probably acceptable to expect callbacks to
> Lancelot> never throw (unfortunately, we can’t have use std::function<void ()
> Lancelot> noexcept> to have this in the type system).  If we accept that callbacks
> Lancelot> can throw, is it OK to skip pending cleanup functions?
> 
> We could catch and ignore exceptions here.
> I'm not sure how important this really is.  The current code has ignored
> it for decades.

My train of thought was
1) This `system ("rm -rf ...")` could be replaced with
std::filesystem::remove_all
2) At least one overload of remove_all can throw
3) What would happen if a cleanup throws (either an existing one, or one
to change/introduce in the future).

I don't think this is a critical issue in any way, just something to
consider in a C -> C++ transition.  The options I can think of are:
- let exceptions go through (some cleanup code might not get a chance to
  execute),
- catch and ignore any exception,
- catch exceptions and re-throw the first one captured after all cleanup
  code has had a chance to run.

I can see pros and cons to each of those, and I don't think I have a
strong preference either way.

FWIW, I skimmed through the rest of the series, and looks reasonable to
me.

Best,
Lancelot.

> 
> Tom