[PATCH] gdb: cast return value of std::unique_ptr::release to void

[PATCH] gdb: cast return value of std::unique_ptr::release to void

[Simon Marchi]

My editor shows warnings like:

     value.c:2784: warning: The value returned by this function should be used
     value.c:2784: note: cast the expression to void to silence this warning [bugprone-unused-return-value]

These warnings come from clangd, so ultimately from one of the clang
static analyzers (probably clang-tidy).

Silence these warnings by casting to void.  Add a comment to explain
why this unusual thing is done.

Change-Id: I58323959c0baf9f1b20a8d596e4c58dc77c6809a
---
 gdb/value.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/gdb/value.c b/gdb/value.c
index 4be408e68702..a325b5ce5523 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -2781,9 +2781,12 @@ add_internal_function (gdb::unique_xmalloc_ptr<char> &&name,
 {
   struct cmd_list_element *cmd
     = do_add_internal_function (name.get (), doc.get (), handler, cookie);
-  doc.release ();
+
+  /* Manually transfer the ownership of the doc and name strings to CMD by
+     setting the appropriate flags.  */
+  (void) doc.release ();
   cmd->doc_allocated = 1;
-  name.release ();
+  (void) name.release ();
   cmd->name_allocated = 1;
 }
 
-- 
2.39.1

Re: [PATCH] gdb: cast return value of std::unique_ptr::release to void

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> My editor shows warnings like:
Simon>      value.c:2784: warning: The value returned by this function should be used
Simon>      value.c:2784: note: cast the expression to void to silence this warning [bugprone-unused-return-value]

Simon> These warnings come from clangd, so ultimately from one of the clang
Simon> static analyzers (probably clang-tidy).

Simon> Silence these warnings by casting to void.  Add a comment to explain
Simon> why this unusual thing is done.

Normally I think we should work to get rid of .release() calls.
However this one looks like a pain, probably why it is already there.
And, your patch doesn't make anything worse in this regard.  So as much
as I hate cast to void I think it's ok.

Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH] gdb: cast return value of std::unique_ptr::release to void

[Simon Marchi]

> Normally I think we should work to get rid of .release() calls.
> However this one looks like a pain, probably why it is already there.
> And, your patch doesn't make anything worse in this regard.  So as much
> as I hate cast to void I think it's ok.

Same feeling here.  It's not pretty, but the (void) doesn't change
anything in what we're doing.

I wanted to change cmd_list_element in the past to have "doc_holder" and
"name_holder" fields, both of which would be
gdb::unique_xmalloc_ptr<char>.  They would be set if the
cmd_list_element owns the memory for doc and name, and would make the
management automatic.  But I'm afraid that people will say "but it makes
cmd_list_element larger for nothing", so I never sent it.  But if you
think it's worth it, I can do that.


> Approved-By: Tom Tromey <tom@tromey.com>

Thanks, will push.

Simon

Re: [PATCH] gdb: cast return value of std::unique_ptr::release to void

[Tom Tromey]

Simon> I wanted to change cmd_list_element in the past to have "doc_holder" and
Simon> "name_holder" fields, both of which would be
Simon> gdb::unique_xmalloc_ptr<char>.  They would be set if the
Simon> cmd_list_element owns the memory for doc and name, and would make the
Simon> management automatic.  But I'm afraid that people will say "but it makes
Simon> cmd_list_element larger for nothing", so I never sent it.  But if you
Simon> think it's worth it, I can do that.

I am not worried about the space savings here.  It doesn't seem so bad
to have a bit of manual management in this code, but at the same time,
it wouldn't be a huge amount of memory, and if it makes things clearer,
then it seems great.

Stove-piping the unique ptr through all this code seems like a pain,
though.

I've sometimes wondered if we want a either-owns-or-is-constant-string
smart pointer type.  But it seems maybe hard to make this safe to use,
as in it may be easy to construct one improperly.

Tom