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

[Jonathan Wakely <jwakely@redhat.com>]

On Mon, 6 Mar 2023 at 11:01, Richard Biener <rguenther@suse.de> wrote:
>
> On Mon, 6 Mar 2023, Jonathan Wakely wrote:
>
> > 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.
>
> OK, it might be OK to mpfr_clear() twice and/or mpfr_clear/mpfr_init
> again.  Quite possibly mpfr_init should get the same treatmen, mixing
> auto_* with explicit lifetime management is bad.

Ah yes, good point.

> > > +
> > > +  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.
>
> Thanks a lot, I've adjusted the patch to the one below.

LGTM.