On 03/01/2013 11:50 PM, Tom Tromey wrote:
>
> Yao> +      /* xfree (regs); */
>
> It seems like this should be deleted.
> I don't really know, though.

It can be removed.

>
> Yao> +	  unsigned short mlen;
>
> I'm curious about the rationale for the type here.

Its type should "uint16_t", because ....

> And a bit later:
>
> Yao> +	  mlen = (uint16_t) bt_ctf_get_uint64 (def);
>
> Why use uint16_t here and not mlen's real type?
> But I don't understand why this has a shorter type anyhow.
>

... the length of contents is 2-bytes, so we choose "uint16_t" for its type.

> Yao> +  /* It's unduly pedantic to refuse to look at the executable for
> Yao> +     read-only pieces; so do the equivalent of readonly regions aka
> Yao> +     QTro packet.  */
> Yao> +  /* FIXME account for relocation at some point.  */
>
> There's a rule against adding new FIXME comments.  Sometimes I think
> maybe we're too strict about this; though in practice it does seem to me
> that these are rarely fixed for their own sake, but instead just as a
> side effect of something else.

Some code are copied from the tfile target (tracepoint.c).  This FIXME 
(and the one below) is copied from tfile target as well.  My original 
plan is to share some code between tfile target and ctf target after ctf 
target patch goes in, as both of them are about reading trace data from 
file.  So I didn't pay much attention on these comments.

>
> I suppose you could rephrase it to be a note.
> Or is it fixable?  It isn't clear to me in what situation this code is
> invoked.
>

Neither do I.  It is beyond my knowledge to give an accurate comment or 
note here, so I remove this FIXME.  My next step is to get rid of the 
duplication (including these comments and FIXMEs) of tfile target and 
ctf target.  We may need other patches to address these FIXMEs separately.

> I was curious why this looks directly at the exec_bfd rather than using
> something like ALL_OBJFILE_OSECTIONS.  It seemed like this might help
> with the relocation problem.
>
> Yao> +  if (exec_bfd)
>
> A while ago we agreed to use the more explicit `exec_bfd != NULL' form.

Fixed.

>
> Yao> +  /* Didn't find anything.  */
> Yao> +  return found;
>
> I think this can be reached in the "did find something" case as well, so
> I suggest just dropping the comment.

This line is removed.

-- 
Yao (齐尧)