Re: [PATCH,V4 1/2] CTF: fix incorrect function return type

[Wei-min Pan <weimin.pan@oracle.com>]

On 5/7/2021 1:07 PM, Tom Tromey wrote:
>>>>>> ">" == Weimin Pan via Gdb-patches <gdb-patches@sourceware.org> writes:
>>> The problems can be illustrated, with any program, below:
>>> (gdb) print main
>>> $1 = {main} 0x0
> Hi.  Sorry about the delay in my re-review.
> It's fine (and expected) to send patch pings to the list.
> It helps keep us honest I guess.
Sorry about that. Shall follow the protocol by pinging the patch going 
forward.

>
>>> -      sym->compute_and_set_names (name.get (), true, objfile->per_bfd);
>>> +      sym->compute_and_set_names (name, true, objfile->per_bfd);
> If the lifetime of 'name' is ok, then you don't need to copy it here --
> so you can pass 'false' as the second parameter.  See the comment in
> symtab.c just before general_symbol_info::compute_and_set_names.

Thanks - passing 'false' as the second parameter to compute_and_set_names is
the right thing to do. All CTF tests are still passing with the change.

>>> +  name = ctf_type_name_raw (fp, tid);
>>> +  if (name == nullptr || strlen (name) == 0)
>>> +  {
> The braces (and the code they surround) look under-indented to me.

Corrected.

>>> -  ccp->pst->add_psymbol (name.get (), true,
>>> +  ccp->pst->add_psymbol (name, true,
> Same here.

Sorry, the only place I can find is here and it looks indented correctly?

-  if (name == nullptr || strlen (name.get ()) == 0)
+  const char *name = ctf_type_name_raw (ccp->fp, tid);
+  if (name == nullptr || strlen (name) == 0)
      return 0;

-  ccp->pst->add_psymbol (name.get (), true,
+  ccp->pst->add_psymbol (name, true,

>
>>> -      pst->add_psymbol (tname.get (), true,
>>> +      pst->add_psymbol (tname, true,
> And here.

Also the indentation looks correct?

        else
         aclass = LOC_TYPEDEF;

-      pst->add_psymbol (tname.get (), true,
+      pst->add_psymbol (tname, true,

>
> If you fix these up, and it still works, it is fine to push.
> If it doesn't work then that would tend to put some doubt on the other
> parts of the patch that don't copy names.

OK, thank you.

>
> thanks,
> Tom