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

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

On 2017-08-08 21:47, Tom Tromey wrote:
> > > > > > "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
>
> Simon> There are many things to fix before the poison-xnew patch can go 
> in,
> Simon> but I think the issue you found is worth fixing sooner than 
> later.
>
> Alright, here's a new version.
>
> Tom
>
> commit 2d47b197465b640fde68194b3ffe0c5dfa57c134
> Author: Tom Tromey <tom@tromey.com>
> Date:   Sat Aug 5 16:40:56 2017 -0600
>
>     C++-ify skip.c
>
>     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.
>
>     ChangeLog
>     2017-08-08  Tom Tromey  <tom@tromey.com>
>
>             * skip.c (skiplist_entry): New constructor.
>             (skiplist_entry::enabled, 
> skiplist_entry::function_is_regexp)
>             (skiplist_entry::file_is_glob): Now bool.
>             (skiplist_entry::file, skiplist_entry::function): Now
>             std::string.
>             (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)
>             (skip_form_bytes, compile_skip_regexp, skip_command, 
> skip_info)
>             (skip_file_p, skip_gfile_p, skip_function_p, 
> skip_rfunction_p)
>             (function_name_is_marked_for_skip): Update.
>             (skip_delete_command): Update.  Use delete.
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 722fade..643e407 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,19 @@
> +2017-08-08  Tom Tromey  <tom@tromey.com>
> +
> +	* skip.c (skiplist_entry): New constructor.
> +	(skiplist_entry::enabled, skiplist_entry::function_is_regexp)
> +	(skiplist_entry::file_is_glob): Now bool.
> +	(skiplist_entry::file, skiplist_entry::function): Now
> +	std::string.
> +	(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)
> +	(skip_form_bytes, compile_skip_regexp, skip_command, skip_info)
> +	(skip_file_p, skip_gfile_p, skip_function_p, skip_rfunction_p)
> +	(function_name_is_marked_for_skip): 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..66e9282 100644
> --- a/gdb/skip.c
> +++ b/gdb/skip.c
> @@ -38,34 +38,44 @@
>
>  struct skiplist_entry
>  {
> +  skiplist_entry (bool file_is_glob_, std::string &&file_,
> +		  bool function_is_regexp_, std::string &&function_)
> +    : number (-1),
> +      file_is_glob (file_is_glob_),
> +      file (file_),
> +      function_is_regexp (function_is_regexp_),
> +      function (function_),

I think you have to use std::move here to avoid a copy being made.  I 
wasn't sure so I made a small test if you want to try for yourself (or 
maybe find that I got it wrong): http://paste.ubuntu.com/25275614/

Otherwise, a simpler way would be to leave the constructor (and calling 
functions) accepting const char* and just change the fields themselves 
to std::string.  The string objects will be constructed using the const 
char* constructor when the skiplist_entry object itself will be 
constructed, so you know you for sure you won't have any unnecessary 
copies.  That would avoid complicating the calling functions as well.  
Either way (adding std::moves or this) are fine for me, it's as you 
wish.

Thanks,

Simon