From: Joel Brobecker <brobecker@adacore.com>
To: Xavier Roirand <roirand@adacore.com>
Cc: gdb-patches@sourceware.org
Subject: Re: [RFA] (Ada) fix breakpoint add on inlined function using function name.
Date: Wed, 20 Dec 2017 11:51:00 -0000 [thread overview]
Message-ID: <20171220115126.tohoo5zpbzzmme2s@adacore.com> (raw)
In-Reply-To: <20171220112322.zh7qawogr6psip53@adacore.com>
> Other than that, the patch looks good to me. But give others
> a couple of weeks so they also get a chance to review the patch
> as well. Here is what I propose: Send a v2 patch, with the revision
> log fixed, so we don't forget about fixing (revision logs cannot
> be changed once pushed without rewriting history), mentioning
> that I conditionally OK-ed the patch but asked for us to wait
> another couple of weeks to give everyone some time to look at it
> also.
>
> It wouldn't hurt as well to confirm to us how you tested this patch,
> and on which platform.
A couple of other suggestions:
1. I'd like to suggest that we remove the "(Ada)" from the commit
subject;
Although, the fact that we can only see this issue with Ada
as of today (based on the assumption that inlined_subroutine
DIES are nested inside the subroutine that inlines them,
thus making Ada the only language which has visibility
over those from outside that subroutine) does not change
the fact that the fix is language-agnostic. So it is conceivable
that this becomes relevant to other languages at some point.
2. Add a "DWARF" to the "RFA" tag, so as to make it clear right
away which area of the code this patch touches.
Eg: [RFA/DWARF] fix breakpoint add on inlined function using function name
The goal of these two suggestions is to quick inform the readers and
reviewers that this is not a patch which is purely constrained
to the ada-* modules (which people typically leave to your Ada
maintainer to review), but a patch which touches common core modules
of GDB. I think it has a higher chance of attracting a reviewer's
attention that way.
--
Joel
prev parent reply other threads:[~2017-12-20 11:51 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-12-20 10:22 Xavier Roirand
2017-12-20 11:23 ` Joel Brobecker
2017-12-20 11:51 ` Joel Brobecker [this message]
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=20171220115126.tohoo5zpbzzmme2s@adacore.com \
--to=brobecker@adacore.com \
--cc=gdb-patches@sourceware.org \
--cc=roirand@adacore.com \
/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