Re: [PATCH] Split macro_buffer in two classes, fix Clang build

Simon Marchi
2020-11-06T16:51:36Z

On 2020-11-06 11:20 a.m., Pedro Alves wrote:
> GDB currently fails to build with (at least) Clang 10 and 11, due to:
>
> $ make
> CXX macroexp.o
> ../../src/gdb/macroexp.c:125:3: error: definition of implicit copy constructor for 'macro_buffer' is deprecated because it has a user-declared destructor [-Werror,-Wdeprecated-copy-dtor]
> ~macro_buffer ()
> ^
>
> Now, we could just add the copy constructor, like we already have a
> copy assignment operator. And like that assignment operator, we would
> assert that only shared buffers can be copied from.
>
> However, it is hard to see why only shared buffers need to be copied.
> I mean, it must be true, otherwise macro support would be broken,
> since currently GDB is relying on the default implementation of the
> copy constructor, which just copies the fields, which can't work
> correctly for the non-shared version. Still, it's not easy to tell
> from the code that that is indeed correct, that there isn't some
> corner case that would require copying a non-shared buffer.
>
> Or to put it simply - the tangling of shared and non-shared buffers in
> the same macro_buffer struct makes this structure hard to understand.
>
> My reaction was -- try splitting the macro_buffer class into two
> classes, one for non-shared buffers, and another for shared buffers.
>
> Comments and asserts like these:
>
> ...
> SRC must be a shared buffer; DEST must not be one. */
>
> static void
> scan (struct macro_buffer *dest,
> struct macro_buffer *src,
> struct macro_name_list *no_loop,
> const macro_scope &scope)
> {
> gdb_assert (src->shared);
> gdb_assert (! dest->shared);
>
> ... made me suspect it should be possible. Then after the split it
> should be easier to reimplement either of the classes if we want.
>
> So I decided to try splitting the struct in two distinct types, and
> see where that leads. It turns out that there is really no good
> reason for a single struct, no code that wants to work with either
> shared or non-shared buffers. It's always shared for input being
> parsed, and non-shared for output.
>
> This commit is the result. I named the new classes
> shared_macro_buffer and growable_macro_buffer.
>
> A future direction could be for example to make shared_macro_buffer
> wrap a string_view and growable_macro_buffer a std::string. With that
> in mind, other than text/len, only the 'last_token' field is common to
> both classes. I didn't feel like creating a base class just for that
> single field.

I think at least making growable_macro_buffer::text a
gdb::unique_xmalloc_ptr would be good to do now (maybe as a subsequent
patch), as it would let us delete the destructor and the explicit
DISABLE_COPY_AND_ASSIGN, as well as simplify the release method.

>
> I constified shared_macro_buffer's 'text' field, which of course had
> some knock-on effects, fixed in the patch.
>
> On the original warning issued by Clang -- now it is clear that really
> only the shared version needs copy ctor/assign, so I added a copy
> ctor, implemented on top of the assignment operator:
>
> shared_macro_buffer (const shared_macro_buffer &src)
> {
> *this = src;
> }

Are the copy constructor and assignment operators needed at all? It
seems like we only do trivial copies of all fields.

> @@ -106,45 +71,81 @@ struct macro_buffer
> shared substring. */
> void set_shared (const char *addr, int len_)
> {
> - text = (char *) addr;
> + text = addr;
> len = len_;
> - size = 0;
> - shared = true;
> }

I'm wondering (for a later patch) if we could get rid of the set_shared
method and just have one constructor (the one that has parameters). If
so, it would make it clear that once created, the shared_macro_buffer
objects never change (if that's indeed the case). And that they can
never be "NULL".

Anyway, thanks for doing this, it's been bugging me for a while.

Simon