Hi Simon,

[copying Tom as well, as he helped me a lot with the C++ part
when designing this API]

> Here are some comments.  Most of them are really just suggestions,
> what you have looks good to me.  The suggestions can easily be
> implemented later, so you don't have to re-do your patch series.

Thanks for your thorough review.

I took a bit of time today to look at your comments, and work on them.
I'll fold the trivial changes in v2 of the patch. For the more impactful
changes, I'll make them separate patches at the end of the patch series.
Hopefully that'll simplify the review process both for the existing
patch as well as the proposed changes.

> > +/* Same as gmp_asprintf, but returning a convenient wrapper type.  */
> > +
> > +gdb::unique_xmalloc_ptr<char> gmp_string_asprintf (const char *fmt, ...);
> 
> I don't know how gmp_string_asprintf will be used, but would it make
> sense to make it return an std::string?  Does gmp_sprintf support
> passing NULL as BUF, so that you could replicate what is done in
> string_printf?

I can't remember the exact details behind the choice of using
a unique_xmalloc_ptr, but I think it was to avoid having an extra
malloc/free when going from the memory allocated by gmp_vasprintf
to the data returned to caller.

I don't think this function is expected to be called that often,
so it sounds worth the trouble making the API more C++-y.
I've attached a copy of the patch that does that as "0001-[...]".

> > +/* A class to make it easier to use GMP's mpz_t values within GDB.  */
> > +
> > +struct gdb_mpz
> > +{
> > +  mpz_t val;
> 
> I don't know what's coming in the following patches, but would it work
> to make the field private and only use it through methods?  Having it
> totally encapsulated would make me more confident that the callers don't
> do anything they are not supposed to with it.
> 
> There would need to be methods for arithmetic operations that we use
> (e.g. mpz_add), but we don't have to add methods for all existing
> functions in gmp, just the ones we use.  And I presume we don't use that
> many.

I'm not sure about that. The main purpose of these classes is to automate
the lifetime of the data allocated when creating the underlying GMP objects.
Beyond that, there are a few help routines that are connected as methods
because it makes sense to do so now that we have the classes, but could
have otherwise been just regular functions.

I am not opposed to the idea of making the GMP objects private,
but I don't really see enough benefits...

> > +  gdb_mpz &operator== (gdb_mpz &&other)
> > +  {
> > +    mpz_swap (val, other.val);
> > +    return *this;
> > +  }
> 
> Is this meant to be "operator="?

Nice catch! I think so, and I'll fix if Tom confirms as well.

> > +  /* Set VAL by importing the number stored in the byte buffer (BUF),
> > +     given its size (LEN) and BYTE_ORDER.
> > +
> > +     UNSIGNED_P indicates whether the number has an unsigned type.  */
> > +  void read (const gdb_byte *buf, int len, enum bfd_endian byte_order,
> > +	     bool unsigned_p);
> > +
> > +  /* Write VAL into BUF as a LEN-bytes number with the given BYTE_ORDER.
> > +
> > +     UNSIGNED_P indicates whether the number has an unsigned type.  */
> > +  void write (gdb_byte *buf, int len, enum bfd_endian byte_order,
> > +	      bool unsigned_p) const;
> 
> These two would be good candidates for gdb::array_view.

If we change these, I think it would make sense to change the
read_fixed_point/write_fixed_point duo as well.

I'm on the fence about this suggestion. Attached is the patch
implementing it. On the plus side, the API is more idiomatic-C++.
One the down side, it very so slightly increases the complexity
of both the implementation and the current callers. It's because
the rest of the infrastructure is still structured in a way that
what we have access to are pointers and sizes. Perhaps one could
say that, even though the point of call is currently a bit more
work, it does help convey the idea that the "len" is the len of
the buffer.

I don't have a strong opinion either way, so I'm happy to follow
what people think. The patch will be included in v2 of the patch
series, and I'm happy to keep or drop.

-- 
Joel