public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Yao Qi <qiyaoltc@gmail.com>
To: Joel Brobecker <brobecker@adacore.com>
Cc: Keven Boell <keven.boell@linux.intel.com>,
	 Yao Qi <qiyaoltc@gmail.com>,
	 gdb-patches@sourceware.org
Subject: Re: [PATCH 1/2] fort_dyn_array: add basic fortran dyn array support
Date: Fri, 22 Jan 2016 15:25:00 -0000	[thread overview]
Message-ID: <86y4bhr5z8.fsf@gmail.com> (raw)
In-Reply-To: <20160122124050.GG5146@adacore.com> (Joel Brobecker's message of	"Fri, 22 Jan 2016 16:40:50 +0400")

Joel Brobecker <brobecker@adacore.com> writes:

> I don't think that reverting without more investigation is a good
> idea. I quickly looked at a good dozen of those reports, and, as
> far as I know, these are new FAILs, so we don't know that they are
> regressions. I could also be compiler issues.

They are fails of tests for the new feature, so they aren't
regressions.  It could be compiler issues, of course.

>
> There is also the fact that reviewing these patches took me not
> just hours, but actually days in total (I don't know how much time
> Keven et al also spent answering all my comments). I would prefer
> re-doing all that work only if we have confirmation that this is
> causing a critical problem that we can't fix without Keven's help.

Your review efforts are not lost if the patch is reverted.  Keven or
some one else will work on the basis of it, so everything, such as
comments and code style, should be still there.

The reviewing efforts shouldn't justify keeping something in GDB, which
breaks tests at the beginning.  Patches are reviewed in the scope of
reviewers' knowledge and experience, so nobody knows the effects of
patches on the complicated software, such as GDB, on some certain env.
It is reasonable to me that patches still cause regressions after
several rounds of careful reviews, but it doesn't mean we should take
them in source tree.

>
> That being said - it doesn't bode well for the future of this
> feature if the authors don't have time to look into issues they
> create...

Yes, I agree.

In short, I am not strongly against this patch as it doesn't cause
critical problem, and I can live up with it.  However, it would be nice
to revert it.

-- 
Yao (齐尧)


P.S.

  "A feature which is omitted can always be added later, when its design
and its implications are well understood.  A feature which is included
before it is fully understood can never be removed later."

  -- C.A.R. Hoare, The Emperor's Old Clothes, 1980 Turing Award Lecture

  reply	other threads:[~2016-01-22 15:25 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-07-01 12:42 [PATCH 0/2] fort_dyn_array: Enable basic Fortran dynamic " Keven Boell
2015-07-01 12:42 ` [PATCH 1/2] fort_dyn_array: add basic fortran dyn " Keven Boell
2015-07-21 18:05   ` Joel Brobecker
2015-08-05 13:41     ` Keven Boell
2015-08-05 13:47     ` Keven Boell
2015-08-05 20:23       ` Joel Brobecker
2015-08-06 11:42         ` keven.boell
2015-08-20 12:52           ` Joel Brobecker
2015-10-09 11:37             ` Keven Boell
2015-10-22  9:59               ` Joel Brobecker
2016-01-20 10:19               ` Yao Qi
2016-01-22  7:22                 ` Keven Boell
2016-01-22 12:40                   ` Joel Brobecker
2016-01-22 15:25                     ` Yao Qi [this message]
2016-01-26 12:34                       ` Joel Brobecker
     [not found]             ` <5613D0DC.3040908@linux.intel.com>
2015-10-22  9:59               ` Joel Brobecker
2015-07-01 12:42 ` [PATCH 2/2] fort_dyn_array: add basic test coverage Keven Boell
2015-07-21 18:19   ` Joel Brobecker
2015-08-05 13:41     ` Keven Boell
2015-10-28 15:30 ` off-trunk gdb.fortran/dwarf-stride.exp no longer PASSes [Re: [PATCH 0/2] fort_dyn_array: Enable basic Fortran dynamic array support] Jan Kratochvil
2015-10-28 16:19   ` Jan Kratochvil
2015-10-28 18:54     ` Joel Brobecker
2015-10-30 10:39       ` Jan Kratochvil
2015-11-04 21:48         ` Joel Brobecker
2015-11-04 22:08           ` Jan Kratochvil
2015-11-05  7:31             ` Keven Boell
2015-11-05  8:22               ` Jan Kratochvil
2015-11-05 16:05                 ` Keven Boell
2016-02-08 13:47 [PATCH 1/2] fort_dyn_array: add basic fortran dyn array support Walfred Tedeschi
2016-02-09 11:36 ` Joel Brobecker
2016-02-09 15:14   ` Walfred Tedeschi

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=86y4bhr5z8.fsf@gmail.com \
    --to=qiyaoltc@gmail.com \
    --cc=brobecker@adacore.com \
    --cc=gdb-patches@sourceware.org \
    --cc=keven.boell@linux.intel.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;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).