Re: [RFA] Expand "info record"

[Pedro Alves <pedro@codesourcery.com>]

On Wednesday 21 October 2009 02:31:41, Michael Snyder wrote:
> Pedro Alves wrote:
> > On Tuesday 20 October 2009 23:25:30, Michael Snyder wrote:
> > 
> >>>> +      /* Display instruction number for last instruction in the log.  */
> >>>> +      printf_filtered (_("Highest recorded instruction number is %llu.\n"), 
> >>>> +                      record_insn_count ? record_insn_count - 1 : 0);
> >>> Why the conditional subtraction?
> >> Because I don't want it to say "-1".
> > 
> > Okay, that much is obvious, but how can you reach here
> > with record_insn_count == 0, given that you check if you
> > have a log at all a bit above?
> 
> Maybe not -- but I'm a belt-and-suspenders guy.
> I don't believe in not checking for something just because
> it "can't happen".  What if somebody changed the check above?

That's why we comment non-obvious code, or add
gdb_asserts (could be overkill), or warnings.  The worse that can
happen if somebody changes the check above, is the new bug is
quickly exposed because gdb starting printing '(ULONGEST) -1'.
Masking a buglet like that is wrong, IMO.

> Yeah, you've convinced me.  Thanks for the prodding,
> and please see new revision.  I'll make that conditional
> go away too.   ;-)

Thanks.  I could nit some more, but the all the info seems
there.  ;-)  I'm fine with the patch.  

I don't see "info record" mentioned in the docs.  Only:
 @kindex info record insn-number
 @item info record insn-number
 Show the current number of recorded instructions.

Was there a corresponding documentation patch?

FTR, it still pains me when I see this:
> +  if (current_target.to_stratum == record_stratum)
> +    {

especially more so now that we have a stratum higher
than record_statum...

-- 
Pedro Alves