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

[Simon Marchi <simark@simark.ca>]

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