Re: [dpdk-dev] tools brainstorming

[Neil Horman <nhorman@tuxdriver.com>]

On Thu, Apr 09, 2015 at 09:29:14PM +0000, Wiles, Keith wrote:
> 
> 
> 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?
This is true, but the typical attitude in the linux kernel is that a change must
have a use case and a user to be considered.  I don't want to dissuade you from
trying to get it accepted, but my concern would be that, since the kernel folks
don't allow the '//' comments, they won't be interested in maintaining the
change.

Neil

> >
> 
>