All of lore.kernel.org
 help / color / mirror / Atom feed
From: Tiwei Bie <tiwei.bie@intel.com>
To: Thomas Monjalon <thomas@monjalon.net>
Cc: Jens Freimann <jfreimann@redhat.com>,
	"Van Haaren, Harry" <harry.van.haaren@intel.com>,
	dev@dpdk.org
Subject: Re: [PATCH] all: refactor coding style
Date: Thu, 20 Jul 2017 17:01:25 +0800	[thread overview]
Message-ID: <20170720090125.GA3807@debian-ZGViaWFuCg> (raw)
In-Reply-To: <1607182.kpCMdDpRBB@xps>

On Thu, Jul 20, 2017 at 11:32:18AM +0300, Thomas Monjalon wrote:
> 20/07/2017 10:56, Jens Freimann:
> > On Wed, Jul 19, 2017 at 06:23:21PM +0800, Tiwei Bie wrote:
> > >On Wed, Jul 19, 2017 at 05:24:38PM +0800, Van Haaren, Harry wrote:
> > [...]
> > > > Hi Tiwei,
> > > >
> > > > Although the idea and motivation for code-cleanup are good, performing
> > > > large cleanup across a code-base is not a good solution. The reason that
> > > > these types of cleanups (or even re-formatting the entire codebase) are not
> > > > performed often is that it "invalidates" any currently-in-progress patch-sets.
> > > > As a result, more work is required from many contributors to rebase useful
> > > > features due to across-the-board white-space cleanups.
> > > >
> > > > Just expressing concern that we need to think carefully about the impacts
> > > > of such a patch.
> > > >
> > >
> > > Yeah, I agree. Such patch may cause many conflicts. But this patch
> > > is almost generated automatically, that is to say, it's a quick work.
> > > And it's more like some fixes (for the bad coding style) rather than
> > > silly re-formatting done by `indent'. So I just want to share it with
> > > the community, and see the potential feedbacks. Thank you for your
> > > comments! :)
> > 
> > what I'm more concerned about with these kind of huge clean-ups is
> > that it makes git-blame less useful for me. Next time I want to look
> > up who changed this line I'll just find your cleanup patch. Then I have
> > to do another step to find out which commit introduced the change I'm
> > looking for. 
> > 
> > I'm more for cleaning up these things next time you do a semantic
> > change in this code.
> 
> +1 for doing clean-up when refactoring code

Hi Jens and Thomas,

I agree with your concerns.

But if you look into this patch, you will find that it's not a
huge cleanup. Actually in this patch, although the file list is
long, there are only 69 lines are changed (across the whole DPDK
source code), and only 1 new line is added. The changes in each
file are very minimal. I don't think it will destroy the useful
info you need in git-blame.

I definitely agree that it would be perfect to clean-up the code
when you need to do a semantic change in this code. But you will
also find that it's very possible that you won't need to do a
semantic change to these code for a very long time. And much of
the changed code in this patch is old code. I think the new code
for DPDK has much better quality than before. It's really annoying
(at least to me) each time come across those bad code. :-(

Thank you for sharing your thoughts! :-)

Best regards,
Tiwei Bie 

  reply	other threads:[~2017-07-20  9:00 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-07-19  9:06 [PATCH] all: refactor coding style Tiwei Bie
2017-07-19  9:24 ` Van Haaren, Harry
2017-07-19 10:23   ` Tiwei Bie
2017-07-20  7:56     ` Jens Freimann
2017-07-20  8:32       ` Thomas Monjalon
2017-07-20  9:01         ` Tiwei Bie [this message]
2017-07-19 10:45 ` Trahe, Fiona
2017-07-20  5:04 ` Shreyansh Jain
2017-07-20  5:53   ` Tiwei Bie
2017-07-20  7:13     ` Shreyansh Jain

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=20170720090125.GA3807@debian-ZGViaWFuCg \
    --to=tiwei.bie@intel.com \
    --cc=dev@dpdk.org \
    --cc=harry.van.haaren@intel.com \
    --cc=jfreimann@redhat.com \
    --cc=thomas@monjalon.net \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.