DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Wiles, Keith" <keith.wiles@intel.com>
To: Stephen Hemminger <stephen@networkplumber.org>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] tools brainstorming
Date: Thu, 9 Apr 2015 21:29:14 +0000	[thread overview]
Message-ID: <D14C58D8.1AA49%keith.wiles@intel.com> (raw)
In-Reply-To: <20150409142349.5b0b3293@urahara>



On 4/9/15, 4:23 PM, "Stephen Hemminger" <stephen@networkplumber.org> wrote:

>On Thu, 9 Apr 2015 21:10:19 +0000
>"Wiles, Keith" <keith.wiles@intel.com> wrote:
>
>> 
>> 
>> On 4/9/15, 2:38 PM, "Jay Rolette" <rolette@infiniteio.com> wrote:
>> 
>> >On Thu, Apr 9, 2015 at 2:16 PM, Neil Horman <nhorman@tuxdriver.com>
>>wrote:
>> >
>> >> On Thu, Apr 09, 2015 at 11:31:39AM -0500, Jay Rolette wrote:
>> >> > On Wed, Apr 8, 2015 at 5:38 PM, Stephen Hemminger <
>> >> > stephen@networkplumber.org> wrote:
>> >> >
>> >> > > On Wed, 8 Apr 2015 16:29:54 -0600
>> >> > > Jay Rolette <rolette@infiniteio.com> wrote:
>> >> > >
>> >> > > > "C comments" includes //, right? It's been part of the C
>>standard
>> >> for a
>> >> > > long time now...
>> >> > >
>> >> > > Yes but.
>> >> > > I like to use checkpatch and checkpatch enforces kernel style
>>which
>> >> does
>> >> > > not allow // for
>> >> > > comments.
>> >> > >
>> >> >
>> >> > Fork checkpatch and disable that bit? DPDK isn't the kernel, so no
>> >> > requirement to follow all of its rules
>> >> >
>> >>
>> >> Doesn't that beg the question, why?  I understand the DPDK isn't the
>> >> kernel, but
>> >> we're not talking about clarity of code, not anything functional to
>>that
>> >> code.
>> >> It seems we would be better served by just taking something that
>>works
>> >>here
>> >> rather than re-inventing the wheel and digging into the minuate of
>>what
>> >> type of
>> >> comments should be allowed (unless there is a compelling reason to
>> >>change
>> >> it
>> >> that supercedes the avilable tools).  If not checkpath, then some
>>other
>> >> tool,
>> >> but It seems to me that coding style is one of those things where we
>>can
>> >> bend to
>> >> the tool rather than taking the time to make the tool do exactly
>>whats
>> >> desired,
>> >> at least until someone gets the time to modify it.
>> >>
>> >
>> >Fair question.
>> >
>> >It depends a bit on how much you want to encourage patch
>>contributions. Is
>> >it worth adding more pain for folks trying to contribute patches for
>> >things
>> >like this?
>> >
>> >Should we force someone to spend time redoing a patch because of which
>>way
>> >they do their parenthesis? What about number of spaces to indent code?
>>//
>> >vs /* */ comments? None of these matter functionally and they don't
>>affect
>> >maintenance generally.
>> >
>> >If someone is modifying existing code, then yeah, they should follow
>>the
>> >prevailing style (indention level, brace alignment, etc.) of the file
>>they
>> >are in. It helps readability, which makes maintenance easier. However,
>> >IMO,
>> >mixing // and /* */ for comments doesn't affect the readability of the
>> >source.
>> >
>> >I know if I submit a patch and the only feedback is that I should have
>> >used
>> >/* */ for comments, I'm extremely unlikely spend extra time to resubmit
>> >the
>> >patch for pedantry.
>> 
>> I looked at checkpatch.pl for few minutes and the code does check for
>>C99
>> comments and adding a command line option to allow C99 comments could
>> pretty simple. I found the code around line 3048 or search for C99, it
>>is
>> possible it could accepted back into Linux as long as the default option
>> was to not allow C99 comments.
>> 
>> Allowing C99 comments would be nice and the only problem I could see if
>> some compiler has a problem with them. I believe all of the compilers we
>> support allow C99 comments.
>> 
>> The only other reason to allow them is if we add some open source code
>>in
>> the future to DPDK which has C99 comments and if would be a pain to have
>> to convert that code every time the open source group released a new
>> version. It does open that path IMO.
>> 
>> Regards,
>> ++Keith
>> >
>> 
>
>But forking a tool means maintaining that tool.

If the tool is pushed back into the main stream, then no you do not have
to maintain it, right?
That was my point and the change a simple one plus I would expect it
should not give anyone a problem unless Linux really wants to stay pre
C99, which is not the case today, right?
>

  reply	other threads:[~2015-04-09 21:29 UTC|newest]

Thread overview: 62+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-20 14:51 Thomas Monjalon
2015-03-20 15:07 ` Butler, Siobhan A
2015-03-23 16:18   ` Thomas Monjalon
2015-03-23 16:50     ` Butler, Siobhan A
2015-03-23 17:35     ` Neil Horman
2015-03-23 23:38     ` Matthew Hall
2015-03-20 15:16 ` Neil Horman
2015-03-23 16:22   ` Jim Thompson
2015-03-23 17:44     ` Neil Horman
2015-03-23 21:56       ` Jim Thompson
2015-03-23 23:01         ` Neil Horman
2015-03-23 16:26   ` Thomas Monjalon
2015-03-20 15:18 ` Simon Kågström
2015-03-23 16:29   ` Thomas Monjalon
2015-03-24  8:31     ` Simon Kågström
2015-03-23  8:41 ` Cao, Waterman
2015-03-23 16:18 ` Mcnamara, John
2015-04-08 10:43 ` Butler, Siobhan A
2015-04-08 11:43   ` Neil Horman
2015-04-08 12:16     ` Butler, Siobhan A
2015-04-08 12:20       ` Butler, Siobhan A
2015-04-08 13:11       ` Neil Horman
2015-04-08 14:40         ` Butler, Siobhan A
2015-04-08 15:39           ` Neil Horman
2015-04-08 22:29           ` Jay Rolette
2015-04-08 22:38             ` Stephen Hemminger
2015-04-09 16:31               ` Jay Rolette
2015-04-09 19:16                 ` Neil Horman
2015-04-09 19:38                   ` Jay Rolette
2015-04-09 20:14                     ` Neil Horman
2015-04-09 21:10                     ` Wiles, Keith
2015-04-09 21:23                       ` Stephen Hemminger
2015-04-09 21:29                         ` Wiles, Keith [this message]
2015-04-10  0:16                           ` Neil Horman
2015-04-10  0:26                       ` Neil Horman
2015-04-10  1:49                         ` Wiles, Keith
2015-04-10 11:41                           ` Neil Horman
2015-04-10 14:43                             ` Wiles, Keith
2015-04-08 14:16   ` Wiles, Keith
2015-04-14 14:50     ` Bruce Richardson
2015-04-08 15:21   ` Wiles, Keith
2015-04-08 15:53   ` Wiles, Keith
2015-04-08 16:16     ` Thomas Monjalon
2015-04-08 16:25       ` Wiles, Keith
2015-04-08 19:54       ` Butler, Siobhan A
2015-04-14 14:21         ` Bruce Richardson
2015-04-14 14:38           ` Neil Horman
2015-04-14 14:47             ` Thomas Monjalon
2015-04-14 14:54               ` Bruce Richardson
2015-04-14 14:52       ` Bruce Richardson
2015-04-14 15:24         ` Thomas Monjalon
2015-04-14 16:19           ` Wiles, Keith
2015-04-14 18:52             ` Wiles, Keith
2015-04-08 18:16   ` Stephen Hemminger
2015-04-08 18:58     ` Matthew Hall
2015-04-08 22:12       ` Stephen Hemminger
2015-04-08 19:51     ` Butler, Siobhan A
2015-04-14 15:29     ` Bruce Richardson
2015-04-08 21:55   ` Don Provan
2015-04-13 15:02   ` Neil Horman
2015-04-13 23:44     ` Stephen Hemminger
2015-04-16 10:49   ` Thomas Monjalon

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=D14C58D8.1AA49%keith.wiles@intel.com \
    --to=keith.wiles@intel.com \
    --cc=dev@dpdk.org \
    --cc=stephen@networkplumber.org \
    /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 NNTP newsgroup(s).