gdb: cast return value of std::unique_ptr::release to void
Commit Message
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(-)
Comments
>>>>> "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
> 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
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
@@ -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;
}