Re: [RFA] Clarify infrun variable naming.

Jim Blandy
2007-12-05T20:59:37Z

Vladimir Prus <vladimir at codesourcery.com> writes:
> On Wednesday 05 December 2007 04:17:52 Jim Blandy wrote:
>>
>> Vladimir Prus <vladimir at codesourcery.com> writes:
>> > The infrun.c file has a variable named trap_expected, which
>> > is a bit misleading -- after all, most times when we resume
>> > inferior, we get SIGTRAP. As it turns out, that variable
>> > is set when we're stepping over breakpoints, so a better
>> > name would be stepping_over_breakpoint. Likewise,
>> > ecs->another_trap also indicates that keep_going should
>> > be stepping over breakpoint. The attached patch clarifies
>> > the naming and adds comments, and has no behaviour changes.
>> > (The patch is on top of my previous breakpoints_inserted
>> > removing patch).
>> > OK?
> ...
>> So, my suggestions were:
>>
>> - We should replace stepping_past_breakpoint and
>> stepping_past_breakpoint_ptid with a single ptid_t variable,
>> deferred_step_ptid.
>>
>> - We should rename trap_expected to stepped_over_breakpoint. The past
>> tense 'stepped' suggests that we're talking about a step which has
>> already happened (which is true by the time we reach
>> handle_inferior_event).
>>
>> (And if I meditate carefully enough on the best possible names, it'll
>> be quite some time before I have to look at Vlad's harder patches. :))
>
> Here's the patch that renamed stepping_past_breakpoint_ptid. I attach both
> patch and the delta relative to the previous revision.
>
> The 'stepped' vs. 'stepping' change was not done -- for reason I've posted
> already.

Sure, that's fine. I agree with your and Dan's arguments.

I noticed some text issues; with those fixed, this can go in.

> +/* Nonzero if we are presenting stepping over breakpoint.

This should be "over a breakpoint".

> -static int trap_expected;
> + If we hit a breakpoint or watchpoint, and then continue,
> + we need to single step the current thread with breakpoints
> + disabled, so that to avoid hitting the same breakpoint or

"so that" needs to be deleted, or perhaps changed to "so as"?

> + watchpoint again. And we should step just a single
> + thread and keep other threads stopped, so that
> + other threads don't miss breakpoints while they are removed.
> +
> + So, this variable simultaneously means that we need to single
> + step current thread, keep other threads stopped, and that
> + breakpoints should be removed while we step.

"step the current thread".

> + This variable is set either:
> + - in proceed, when we resume inferior on explicit user's request

"resume the inferior at the user's explicit request"

> +/* If not equal to null_ptid, means that after stepping over breakpoint

'this means'

> + is finished, we need to switch to deferred_step_ptid, and step it.
> +
> + The use case is when a breakpoint in one thread, and then the user

'when one thread has hit a breakpoint'?

> + has switched to another thread and issued 'step'. We need to step over
> + breakpoint in the thread which hit breakpoint, but then continue

'the breakpoint'

> + stepping the thread user has selected. */
> +static ptid_t deferred_step_ptid;