Split macro_buffer in two classes, fix Clang build
authorPedro Alves <pedro@palves.net>
Fri, 6 Nov 2020 17:19:02 +0000 (17:19 +0000)
committerPedro Alves <pedro@palves.net>
Fri, 6 Nov 2020 17:19:02 +0000 (17:19 +0000)
commitac3d4064096ecd4b012e5db91b1530df5cb4a7cc
treead4c3513a96961ced982880a7148518aa4ce78b5
parent2c72361c810f4b6223949363f635376e4311ac7a
Split macro_buffer in two classes, fix Clang build

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 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 only
the shared version needs to be copied.  Since this class doesn't need
a user-declared destructor, the default implementations of the copy
assign/ctor can be used, and Clang no longer warns.

The growable version doesn't need to be copied, so I disabled
copy/assign for it.

gdb/ChangeLog:

* macroexp.c (struct macro_buffer): Split in two classes.  Add
uses adjusted.
(struct shared_macro_buffer): New, factored out from struct
macro_buffer.
(struct growable_macro_buffer): New, factored out from struct
macro_buffer.
(set_token, get_comment, get_identifier, get_pp_number)
(get_character_constant, get_string_literal, get_punctuator)
(get_next_token_for_substitution): Constify parameters.
(substitute_args): Constify locals.

Change-Id: I5712e30e826d949715703b2e9172adf04e63b152
gdb/ChangeLog
gdb/macroexp.c