[00/20] Remove cleanups

Message ID 20190213212927.9474-1-tom@tromey.com
Headers show
Series
  • Remove cleanups
Related show

Message

Tom Tromey Feb. 13, 2019, 9:29 p.m.
This series removes the remaining uses of make_cleanup from the tree,
then removes make_cleanup and some related functions (but note that
final cleanups remain).

Then, it removes the TRY/CATCH macros in favor of ordinary C++ code.

Finally, it cleans up a few spots that currently use TRY/CATCH but
that are more succinctly written using SCOPE_EXIT or RAII.

Regression tested by the buildbot.

Tom

Comments

John Baldwin Feb. 13, 2019, 9:48 p.m. | #1
On 2/13/19 1:29 PM, Tom Tromey wrote:
> This series removes the remaining uses of make_cleanup from the tree,

> then removes make_cleanup and some related functions (but note that

> final cleanups remain).

> 

> Then, it removes the TRY/CATCH macros in favor of ordinary C++ code.

> 

> Finally, it cleans up a few spots that currently use TRY/CATCH but

> that are more succinctly written using SCOPE_EXIT or RAII.

> 

> Regression tested by the buildbot.


Yay!

The only comment I have is that the structure names for exceptions look a
bit awkward in code now with the mix of lower and upper case, e.g.:

   try
     {
       e_msg = ada_exception_message_1 ();
     }
   catch (struct gdb_exception_RETURN_MASK_ERROR &e)
     {
       e_msg.reset (nullptr);
     }

We could drop the 'struct' perhaps, but not sure if we could simplify the
classes a bit to something like:

struct gdb_exception_error : public gdb_exception
{
};

struct gdb_exception_quit : public gdb_exception
{
};

And use 'gdb_execption' instead of 'struct gdb_exception_RETURN_MASK_ALL',
'gdb_exception_error' instead of 'struct gdb_exception_RETURN_MASK_ERROR',
and 'gdb_exception_quit' instead of 'struct gdb_exception_RETURN_MASK_QUIT'?

-- 
John Baldwin
Tom Tromey Feb. 14, 2019, 2:48 p.m. | #2
>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:


John> The only comment I have is that the structure names for exceptions look a
John> bit awkward in code now with the mix of lower and upper case, e.g.:

That is a good point.  I think I can easily update the exception
rewriter to also rename these classes.  I will take a look.  Plan B is
to just have a second patch to do the renaming.

thanks,
Tom
Pedro Alves Feb. 14, 2019, 3:34 p.m. | #3
On 02/13/2019 09:48 PM, John Baldwin wrote:
> On 2/13/19 1:29 PM, Tom Tromey wrote:

>> This series removes the remaining uses of make_cleanup from the tree,

>> then removes make_cleanup and some related functions (but note that

>> final cleanups remain).

>>

>> Then, it removes the TRY/CATCH macros in favor of ordinary C++ code.

>>

>> Finally, it cleans up a few spots that currently use TRY/CATCH but

>> that are more succinctly written using SCOPE_EXIT or RAII.

>>

>> Regression tested by the buildbot.

> 

> Yay!

> 

> The only comment I have is that the structure names for exceptions look a

> bit awkward in code now with the mix of lower and upper case, e.g.:

> 

>    try

>      {

>        e_msg = ada_exception_message_1 ();

>      }

>    catch (struct gdb_exception_RETURN_MASK_ERROR &e)

>      {

>        e_msg.reset (nullptr);

>      }

> 

> We could drop the 'struct' perhaps, but not sure if we could simplify the

> classes a bit to something like:

> 

> struct gdb_exception_error : public gdb_exception

> {

> };

> 

> struct gdb_exception_quit : public gdb_exception

> {

> };

> 

> And use 'gdb_execption' instead of 'struct gdb_exception_RETURN_MASK_ALL',

> 'gdb_exception_error' instead of 'struct gdb_exception_RETURN_MASK_ERROR',

> and 'gdb_exception_quit' instead of 'struct gdb_exception_RETURN_MASK_QUIT'?


Yes please.  The only reason I originally named those structs that way
was to make the CATCH macro simpler to map in the C++ variant:

 #define CATCH(EXCEPTION, MASK)                                          \
           } while (0);                                                  \
         }                                                               \
     catch (struct gdb_exception ## _ ## MASK &EXCEPTION)

We should also catch by const reference, IMO.  Is there a good reason
for the non-const-ness?

Thanks,
Pedro Alves
Tom Tromey Feb. 14, 2019, 4:35 p.m. | #4
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:


Pedro> We should also catch by const reference, IMO.  Is there a good reason
Pedro> for the non-const-ness?

Just conservatism on my part, but it is easy to do the experiment.

Tom
John Baldwin Feb. 14, 2019, 8:38 p.m. | #5
On 2/14/19 6:48 AM, Tom Tromey wrote:
>>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:

> 

> John> The only comment I have is that the structure names for exceptions look a

> John> bit awkward in code now with the mix of lower and upper case, e.g.:

> 

> That is a good point.  I think I can easily update the exception

> rewriter to also rename these classes.  I will take a look.  Plan B is

> to just have a second patch to do the renaming.


I think a second pass is fine unless its trivial to re-run the script.  I
second Pedro's suggestion about trying 'const' as well.

-- 
John Baldwin
Tom Tromey Feb. 14, 2019, 10:49 p.m. | #6
>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:


John> I think a second pass is fine unless its trivial to re-run the script.  I
John> second Pedro's suggestion about trying 'const' as well.

"const" worked fine.

For the renaming I've opted for a second patch, since it was simpler to
deal with the ChangeLog this way.

I don't plan to check this in until after other things have landed and
the 8.3 branch has been made.

Tom
John Baldwin Feb. 14, 2019, 10:55 p.m. | #7
On 2/14/19 2:49 PM, Tom Tromey wrote:
>>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:

> 

> John> I think a second pass is fine unless its trivial to re-run the script.  I

> John> second Pedro's suggestion about trying 'const' as well.

> 

> "const" worked fine.


\o/

> For the renaming I've opted for a second patch, since it was simpler to

> deal with the ChangeLog this way.

> 

> I don't plan to check this in until after other things have landed and

> the 8.3 branch has been made.


Sounds good to me.

-- 
John Baldwin