Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
From: "Anwar, Ali" <Ali_Anwar@mentor.com>
To: "dje@google.com" <dje@google.com>
Cc: "Qi, Yao" <Yao_Qi@mentor.com>,
	"gdb-patches@sourceware.org"	<gdb-patches@sourceware.org>
Subject: RE: Patch to propagate GDB's knowledge of the executing state to frontend
Date: Fri, 09 Nov 2012 19:31:00 -0000	[thread overview]
Message-ID: <FA4FB4F1B7AB91448E3A0B4F3A885DF496CFC441@EU-MBX-02.mgc.mentorg.com> (raw)
In-Reply-To: <50941519.6010005@codesourcery.com>

Ping... OK to commit?

Regards,
-Ali
________________________________________
From: gdb-patches-owner@sourceware.org [gdb-patches-owner@sourceware.org] on behalf of Anwar, Ali
Sent: Friday, November 02, 2012 11:46 PM
To: dje@google.com
Cc: Qi, Yao; gdb-patches@sourceware.org
Subject: Re: Patch to propagate GDB's knowledge of the executing state to frontend

On 11/02/2012 09:15 PM, dje@google.com wrote:
> Yao Qi writes:
>   >  On 10/25/2012 07:09 PM, ali_anwar wrote:
>   >  >  [...]
>   >  >  @@ -1,3 +1,13 @@
>   >  >  +2012-10-25  Ali Anwar<ali_anwar@codesourcery.com>
>   >  >  +
>   >  >  +     * infrun.c (handle_inferior_event_stub, regcache_dup_stub):
>   >  >  +     New functions.
>   >  >  +     (normal_stop): Change to propagate GDB's knowledge of the
>   >  >  +     executing state to frontend when not able to fetch registers.
>   >  >  +     (wait_for_inferior): Chnage to propagate GDB's knowledge of
>   >                                ^^^^^^ typo
>   >
>   >
>   >  >  +     the executing state if not able to fetch backtrace once the
>   >  >  +     step has already occured.
>   >                            ^^^^^^^ typo.
>   >
>   >  In each changelog entry, we'll put 'what do we change' instead of 'why
>   >  do we change in this way'.  So this entry can be simplified.
>
> Hi.
>
> I agree with your first sentence, and would add that if such an
> explanation is needed, it belongs in the code not the changelog.
> [We don't have enough comments in the code explaining *why* things
> are the way they are.]
>
> But I'd say that's not the case here, at least for the changelog entries.
> Instead, I would remove the leading "Change to", and just say "Propagate ...".
>
> Also, I would add a comment to the code explaining *why* the calls are wrapped
> in catch_error (and I would have the comment live at the call to catch_error,
> not in the definition of the two new stubs).
>
> One could also say the two new functions also require comments,
> but they're pretty simple and hook_stop_stub doesn't have a comment,
> so I'd be ok with leaving them out.

Thanks for the review. Please find attached the modified patch.

-Ali


  reply	other threads:[~2012-11-09 19:31 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-10-25 11:10 ali_anwar
2012-10-30  6:20 ` Yao Qi
2012-10-30 11:53   ` ali_anwar
2012-11-02 10:28     ` ali_anwar
2012-11-02 11:47       ` Yao Qi
2012-11-02 12:24     ` ali_anwar
2012-11-02 16:15   ` dje
2012-11-02 18:47     ` ali_anwar
2012-11-09 19:31       ` Anwar, Ali [this message]
2012-11-09 19:42         ` Pedro Alves
2012-11-09 19:48       ` Pedro Alves
2012-11-09 23:00         ` Luis Machado
2012-11-09 23:02           ` Luis Machado
2012-11-13 21:57           ` Tom Tromey
2012-11-13 22:23             ` Luis Machado
2012-11-14 18:29               ` Pedro Alves
2012-11-15 21:23                 ` Tom Tromey

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=FA4FB4F1B7AB91448E3A0B4F3A885DF496CFC441@EU-MBX-02.mgc.mentorg.com \
    --to=ali_anwar@mentor.com \
    --cc=Yao_Qi@mentor.com \
    --cc=dje@google.com \
    --cc=gdb-patches@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox