Re: [RFA] C++-ify skip.c

[Simon Marchi <simon dot marchi at polymtl dot ca>]

On 2017-08-06 21:42, Tom Tromey wrote:
> I happened to notice that skiplist_entry, in skip.c, contains a
> gdb::optional<compiled_regex> -- but that this object's destructor is
> never run.  This can result in a memory leak.
>
> This patch fixes the bug by applying a bit more C++: changing this
> code to use new and delete, and std::unique_ptr; and removing cleanups
> in the process.
>
> Built and regression tested on x86-64 Fedora 25.

Ah thanks, I tripped on this with me poison-XNEW patch.  I didn't have 
the time to fix and post it yet.

> ChangeLog
> 2017-08-06  Tom Tromey  <tom@tromey.com>
>
> 	* skip.c (skiplist_entry): New constructor.
> 	(~skiplist_entry): New destructor.
> 	(skiplist_entry::enabled): Now bool.
> 	(make_skip_entry): Return a unique_ptr.  Use new.
> 	(free_skiplist_entry, free_skiplist_entry_cleanup)
> 	(make_free_skiplist_entry_cleanup): Remove.
> 	(skip_command, skip_disable_command, add_skiplist_entry): Update.
> 	(skip_delete_command): Update.  Use delete.
> ---
>  gdb/ChangeLog | 11 ++++++++
>  gdb/skip.c    | 89 
> +++++++++++++++++++++++------------------------------------
>  2 files changed, 45 insertions(+), 55 deletions(-)
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 722fade..12e0d02 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,14 @@
> +2017-08-06  Tom Tromey  <tom@tromey.com>
> +
> +	* skip.c (skiplist_entry): New constructor.
> +	(~skiplist_entry): New destructor.
> +	(skiplist_entry::enabled): Now bool.
> +	(make_skip_entry): Return a unique_ptr.  Use new.
> +	(free_skiplist_entry, free_skiplist_entry_cleanup)
> +	(make_free_skiplist_entry_cleanup): Remove.
> +	(skip_command, skip_disable_command, add_skiplist_entry): Update.
> +	(skip_delete_command): Update.  Use delete.
> +
>  2017-08-05  Tom Tromey  <tom@tromey.com>
>
>  	* compile/compile-object-load.c (compile_object_load): Use
> diff --git a/gdb/skip.c b/gdb/skip.c
> index bf44913..f3291f3 100644
> --- a/gdb/skip.c
> +++ b/gdb/skip.c
> @@ -38,6 +38,24 @@
>
>  struct skiplist_entry
>  {
> +  skiplist_entry (int file_is_glob_, const char *file_,
> +		  int function_is_regexp_, const char *function_)
> +    : number (-1),
> +      file_is_glob (file_is_glob_),

file_is_glob and function_is_regexp can probably be changed to bools 
too.

> +      file (file_ == NULL ? NULL : xstrdup (file_)),
> +      function_is_regexp (function_is_regexp_),
> +      function (function_ == NULL ? NULL : xstrdup (function_)),

If we are strdup'ing (making a copy) file and function, we might as well 
store them in std::strings, so we don't need to explicitly xfree them.  
And the implicitly defined copy constructor/assignment operator will 
also work (which is not the case now).  An empty string is probably 
enough to mean the field is not used (which is expressed by a NULL value 
currently).

Thanks,

Simon