Re: [RFA] Expand "info record"

[Michael Snyder <msnyder@vmware.com>]

Pedro Alves wrote:
> 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...

We do?  ...

OK -- how would you suggest doing this sort of test?

I'll get right to work on the docs patch, thanks for reminding me.