Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
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


      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