Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
From: Joel Brobecker <brobecker@adacore.com>
To: Jan Kratochvil <jan.kratochvil@redhat.com>
Cc: gdb-patches@sourceware.org
Subject: Re: [patch 2/4] Fix hw watchpoints: reordered / simultaneously hit 	[fixup #1]
Date: Tue, 03 Nov 2009 01:12:00 -0000	[thread overview]
Message-ID: <20091103011145.GD4557@adacore.com> (raw)
In-Reply-To: <20091012155916.GA20031@host0.dyn.jankratochvil.net>

Jan,

Sorry for the delay on reviewing this...

I'll confess that I am having a hard time following the patch, now,
since we're now trying to achieve three different things: there is
the fact that we want one of the target operations to take a ptid,
and then the fact that we use that opportunity to change the interface,
and (oh yeah!) a bug that we're trying to fix on x86/x86_64. Baby
steps are really easier to follow, at least for me.

> If we should do the upgrade my primary reason for this change is that
> I find the single functionality being split into two target functions
> (to_stopped_by_watchpoint and to_stopped_data_address) to be
> confusing.  Chose a new name to easily be able to keep the old
> deprecated implementations working until its host maintainers can get
> to update them as I cannot even compile some of the host files.

I'm having second thoughts on this one, and I am no longer seeing
much benefit in one interface vs the other (personal opinion, of course).
In fact, the function semantics are not very confusing when you look
at how the functions are used by the callers.  It might be confusing
for the implementation, although that would be very easy to fix by
improved documentation in the target.h code.

That being said, I'm still 50/50 on this.  If other maintainers would
like to merge the two functions, then I would be fine with that.

But I see that such a transition would leave the code in a transitional
state where we have both the old code and the new code co-existing.
The problem is that it does not help maintaining the code while
transitioning, and I fear that transitions of this kind can take
a long time before we complete them.  I do not normally hesitate
making this sort of progressive change when I can see a clear benefit,
but I just don't see a clear-cut benefit right now. Anything I might
have missed?

(I left patch #1 out for now, as it is now dependent on this patch)

-- 
Joel


      parent reply	other threads:[~2009-11-03  1:12 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-08-17 19:46 [patch 2/4] Fix hw watchpoints: reordered / simultaneously hit Jan Kratochvil
2009-10-02 22:13 ` [patch 2/4] Fix hw watchpoints: reordered / simultaneously hit [fixup #1] Jan Kratochvil
2009-10-02 23:01   ` Joel Brobecker
2009-10-03 17:23     ` Jan Kratochvil
2009-10-07 18:39       ` Joel Brobecker
2009-10-12 16:00         ` Jan Kratochvil
2009-10-12 18:58           ` Eli Zaretskii
2009-10-13 16:02           ` Jan Kratochvil
2009-11-03  1:12           ` 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=20091103011145.GD4557@adacore.com \
    --to=brobecker@adacore.com \
    --cc=gdb-patches@sourceware.org \
    --cc=jan.kratochvil@redhat.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