Re: [PATCH] [RFC] RAII auto_mpfr and autp_mpz

[Jonathan Wakely <jwakely@redhat.com>]

On Mon, 6 Mar 2023 at 10:11, Richard Biener <rguenther@suse.de> wrote:
>
> The following adds two RAII classes, one for mpz_t and one for mpfr_t
> making object lifetime management easier.  Both formerly require
> explicit initialization with {mpz,mpfr}_init and release with
> {mpz,mpfr}_clear.
>
> I've converted two example places (where lifetime is trivial).
>
> I've sofar only build cc1 with the change.  Any comments?
>
> Thanks,
> Richard.
>
>         * system.h (class auto_mpz): New,
>         * realmpfr.h (class auto_mpfr): Likewise.
>         * fold-const-call.cc (do_mpfr_arg1): Use auto_mpfr.
>         (do_mpfr_arg2): Likewise.
>         * tree-ssa-loop-niter.cc (bound_difference): Use auto_mpz;
> ---
>  gcc/fold-const-call.cc     |  8 ++------
>  gcc/realmpfr.h             | 15 +++++++++++++++
>  gcc/system.h               | 14 ++++++++++++++
>  gcc/tree-ssa-loop-niter.cc | 10 +---------
>  4 files changed, 32 insertions(+), 15 deletions(-)
>
> diff --git a/gcc/fold-const-call.cc b/gcc/fold-const-call.cc
> index 43819c1f984..fa0b287cc8a 100644
> --- a/gcc/fold-const-call.cc
> +++ b/gcc/fold-const-call.cc
> @@ -130,14 +130,12 @@ do_mpfr_arg1 (real_value *result,
>
>    int prec = format->p;
>    mpfr_rnd_t rnd = format->round_towards_zero ? MPFR_RNDZ : MPFR_RNDN;
> -  mpfr_t m;
>
> -  mpfr_init2 (m, prec);
> +  auto_mpfr m (prec);
>    mpfr_from_real (m, arg, MPFR_RNDN);
>    mpfr_clear_flags ();
>    bool inexact = func (m, m, rnd);
>    bool ok = do_mpfr_ckconv (result, m, inexact, format);
> -  mpfr_clear (m);
>
>    return ok;
>  }
> @@ -224,14 +222,12 @@ do_mpfr_arg2 (real_value *result,
>
>    int prec = format->p;
>    mpfr_rnd_t rnd = format->round_towards_zero ? MPFR_RNDZ : MPFR_RNDN;
> -  mpfr_t m;
>
> -  mpfr_init2 (m, prec);
> +  auto_mpfr m (prec);
>    mpfr_from_real (m, arg1, MPFR_RNDN);
>    mpfr_clear_flags ();
>    bool inexact = func (m, arg0.to_shwi (), m, rnd);
>    bool ok = do_mpfr_ckconv (result, m, inexact, format);
> -  mpfr_clear (m);
>
>    return ok;
>  }
> diff --git a/gcc/realmpfr.h b/gcc/realmpfr.h
> index 5e032c05f25..2db2ecc94d4 100644
> --- a/gcc/realmpfr.h
> +++ b/gcc/realmpfr.h
> @@ -24,6 +24,21 @@
>  #include <mpfr.h>
>  #include <mpc.h>
>
> +class auto_mpfr
> +{
> +public:
> +  auto_mpfr () { mpfr_init (m_mpfr); }
> +  explicit auto_mpfr (mpfr_prec_t prec) { mpfr_init2 (m_mpfr, prec); }
> +  ~auto_mpfr () { mpfr_clear (m_mpfr); }
> +
> +  operator mpfr_t& () { return m_mpfr; }


This implicit conversion makes the following mistake possible, if code
is incorrectly converted to use it:

auto_mpfr m (prec);
// ...
mpfr_clear (m);  // oops!

You could prevent that by adding this to the class body:

friend void mpfr_clear (auto_mpfr&) = delete;

This will be a better match for calls to mpfr_clear(m) than using the
implicit conversion then calling the real function, and will give an
error if used:
auto.cc:20:13: error: use of deleted function 'void mpfr_clear(auto_mpfr&)'

This deleted friend will not be a candidate for calls to mpfr_clear
with an argument of any other type, only for calls with an argument of
type auto_mpfr.

> +
> +  auto_mpfr (const auto_mpfr &) = delete;

This class has an implicit-defined assignment operator, which would
result in a leaks and double-frees.
You should add:
   auto_mpfr &operator=(const auto_mpfr &) = delete;
This ensures it can't becopied by construction or assignment.

The same two comments apply to auto_mpz.