Re: Re: [PATCH v4] Add support for creating new types from the Python API

[Matheus Branco Borella <dark.ryu.550@gmail.com>]

Thanks for the review, I've got a few questions and things to add before
I submit the v5, if that's okay.

Tom Tromey <tom@tromey.com> writes:

> WDYT about "make_" instead?

Yeah, that works. I don't feel particularly strongly about the names
that I picked, I'd mostly just picked them to mirror the internal ones
because I didn't have a better idea. I'll switch to `make_*_type`.

> Making any sort of type without filling in the details is probably a
> recipe for crashes.
> 
> Is there a specific situation you needed this for?

The main thing I had in mind was creating a void type, but yeah, it
makes sense to avoid exposing the generic type creation function without
also providing a proper way to fill in the details, so I'll take that
one out.

> I'm curious whether this one is really needed, because
> gdb.Type.pointer() exists.

Like I've told Eli, the main intent behind having it originally was
that, assuming one knows how to create a properly-sized pointer for
the architecture, one could, without having to rely on any of the type
lookup functions.

That being said, now that I think about it, I don't think there's any
case, when avoiding type lookup for pointer types might be necessary,
that can't be solved just as well by using `gdb.Type.pointer()`, while
also avoiding the footguns associated with `make_pointer_type`. So I'll
take it out, too.

> It seems like this could all just create a type_allocator directly and
> be simpler, like the 'kind' isn't needed.

> Is the none case really possible?
> It might be better to just throw an exception from the constructor or
> during argument validation or something like that.

Most of these fall under the same response, so I'll just reply to them
all at once.

When I was writing this patch, I had the following in mind:
 1st - This patch was first written before GDB switched to C++17, so I
       had no access to std::optional<>.
 2nd - I felt like throwing an exception over doing the `->valid()`
       check explicitly would be less clear about my intent for people
	   reading the code.

The design of `type_storage_owner` follows from those, and I don't feel
like changing it to use std::option<> or exceptions would be much of an
improvement in readability.

Would it really be that much of an improvement?

> I think the uses of this could probably use TYPE_ALLOC instead.

Isn't that only valid for `struct type`? I don't think I follow. Some of
the allocations (and I'm pretty sure at least one has to) happen before
the call to `init_*_type`.