Re: [dpdk-dev] tools brainstorming

["Wiles, Keith" <keith.wiles@intel.com>]

On 4/10/15, 6:41 AM, "Neil Horman" <nhorman@tuxdriver.com> wrote:

>On Fri, Apr 10, 2015 at 01:49:33AM +0000, Wiles, Keith wrote:
>> 
>> 
>> On 4/9/15, 7:26 PM, "Neil Horman" <nhorman@tuxdriver.com> wrote:
>> 
>> >On Thu, Apr 09, 2015 at 09:10:19PM +0000, Wiles, Keith 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.
>> >> 
>> >
>> >So, this again seems to be bad philosophy in my mind.  If we are, to
>>use
>> >your
>> >exmple, accept code into the DPDK in the future with comments that
>> >violate our
>> >selected style, it is then, by definition, in violation of the style
>> >guidelines.
>> 
>> Who stated it violated the style guidelines, we do not have a style
>> guideline yet. This why Shiobhan sent out the guidelines in the first
>> place.
>> 
>No one said anyone is violating our style guidelines, I think thats pretty
>obvious.  You had said that allowing C99 comments would be a nice thing
>to be
>able to do in the future if we were to want to integrate a submission
>that had
>them.  While thats true, I was pointing out that what you were suggesting
>was
>asserting the notion that doing so was relaxing style guidelines to allow
>for
>easier acceptance, while style guidelines (as a general notion, not as any
>specific set of rules we currently do or may in the future assert) are
>there to
>enforce a consistent look in the code.
>
>Put another way, we can certainly allow for both styles of comment (or
>multiple
>styles of any aspect of the code), if we want to, but doing so is somewhat
>against the notion of a 'style', as a style provides a consistent look,
>and
>making the argument that we allow both because it makes for easier code
>submission and acceptance is a bit backwards.
>
>Note, I'm not saying that we can't accept both types of comments (or
>multiple
>types of any kind of style aspect), only that, if we are to consider
>multiple
>types of any style aspect, we should decide based on how it will make the
>code
>look, not on how much harder it will be to get the code accepted during
>review.
>
>1) If code acceptance is the goal, we should have no style guideline
>
>2) If pretty, easy to read code is the goal, we should make the style
>fairly
>spicific with very limited options for style choices
>
>If the goal is a happy medium, we should make sure that we have a tool
>that
>makes (2) easy to achive so that at review time on the list, style is a
>non-issue.
>
>> >If we accept it anyway, or if we allow both styles (by documenting
>> >it/codifying
>> >it a tool to check for/etc) then we dilute the style guide.  Maybe in
>>some
>> >cases, such as this, thats ok, but its something to be cogniscent of.
>> >Especially if making the choice to allow both put us in a position of
>> >having to
>> >maintain a tool to do the checking, then I think we need to fall on the
>> >side of
>> >going with what the tool (checkpatch or something else) does, unless we
>> >have a
>> >maintainer stepping up.
>> 
>> Sure maintaining a tool for the C99 comment seems pretty easy, if no one
>> want to maintain the tool that is another problem.
>If this is you stepping up for that role, then we're good to go, that
>means we
>can have a tool that supports any style we want to implement.
>
>> Whether to support C99
>> comments is the point not that we have to maintain the tool. At this
>>point
>> checkpatch is a OK tool if you only want Linux kernel coding style and
>>at
>> this point we are not Linux code style. It just so happens the
>>checkpatch
>> provides some checks we seem to agree with.
>> 
>You're right here, we should be deciding if we want to support C99
>comments or
>not.  But so far doing so has the implication of needing to
>maintain/aquire/write a tool to do so.  If some other tool allows for the
>style
>we choose, so be it.  But so far it seems like we havent found a tool
>that does
>that in a way we are generally happy with, and so the question of how to
>enforce
>the style has remained hanging out there.  But yes, if we have a tool
>that can
>be configured for this, we can stop talking about the tooling aspect.
>
>> If we really want some type of tool to check every detail of Œour¹
>>coding
>> style then we most likely need to stop using checkpatch IMO for a tool
>> like astyle, ununcrustify or some tool that provides the best solution.
>> 
>Yes, we do, we have to, otherwise style never gets enforced.  Though
>astyle and
>uncrusity are beautifiers, not checkers.  While I'm not opposed to a tool
>that
>formats the code for you, I think we need to require that it be able to
>operate
>in a dry-run mode, where it simply points out your formatting deviations,
>so
>that you can make directions, or decide you need to violate the
>guidelines.
>
>> I was suggesting we could use one of these tool to create a common
>>config
>> file for everyone to use and then checkpatch may not be required. At
>>least
>> these other tools beside check patch seem to have a huge number of
>>options
>> to format the automatically instead of the developer having to do that
>> work by hand.
>Thats fine, as long as it has a mode whereby it just tells you whats
>wrong, and
>doesn't fix it for you.
>
>> >
>> >The bottom line is that style guides enforce style, and tooling makes
>> >contributors condusive to following the style.  If we have someone
>>that is
>> >willing to maintain such a tool, then we have a lot of leway in what
>>the
>> >style
>> >is, but if we don't then we really need to follow the style that an
>> >existing
>> >tool provides, because without tooling, contributors aren't likely to
>> >bother
>> >with strict adherence to the style.
>> 
>> For tools like astyle or uncrustify we do not have to maintain those
>>tools
>> as they are already being maintained, we just have to use them. It
>>seems a
>> waste to me we are not willing to change because someone may have to
>>make
>> a single change to checkpatch, which we may never have to change again
>> buying we can not get it upstreamed. It is possible we can drop the use
>>of
>> checkpatch and use one of the other tools made just for style
>>formatting.
>> 
>I'm not opposed to making a change to checkpatch, we just need to know
>whos
>going to do it and maintain it if Linus doesn't accept it.  If thats you,
>or you
>know someone who will step up, we're good to go.
>
>> To me checkpatch is a tool that needs to be replaced by tools that are
>> made for code formatting, not just checking. I would assume checkpatch
>>has
>> its place in the Linux kernel and it needs to be used only by the Linux
>> Kernel. 
>Thats right, but theres no reason we can't adopt it.  Not that we have
>to, but
>it seems like we already have lots of code that follows its style.
>
>> I believe it we took the time to format the code to one of the
>> tools and require a developer to use that tool with the provided config
>> file it would be simpler to enforce the coding style.
>> 
>Question: What about the kernel code that we borrow from the upstream
>projects
>(igb/e1000/ixgbe kernel code)?  Are you proposing an automatic
>reformatting
>there?  If we have expectations of integrating future kernel code, it
>might be
>worthwhile adopting the kernel style (at least for those files).  I
>realize
>thats exactly the argument I made in opposition above (that we shouldn't
>select
>style based on code submissions), but I wanted to point it out.  Or are we
>exempting kernel code from this?
>
>
>> As we patch a file the tool could be applied before the commit. It will
>> effect other patches to that file, but we have to do merges today and it
>> will not change anytime soon. If you applied a tool to format all of the
>> code at some flag day, we would have a bit of churn then it would be
>>over.
>> 
>This is true, as noted above, I'm fine with some other tool, but I would
>like to
>see it have an informational mode, where it just points out errors in
>style
>rather than automatically correcting them.

We both know a coding style is needed, what the coding style is like needs
to be defined and we need to focus on Siobhan document instead of
discussing the these distracting issues. We both seem to want a tool, lets
discuss it after we define the coding style.

Keith
>
>> I would suggest we focus on the coding style and see what it means to
>>the
>> tool/checkpatch. Read the email from Siobhan and then we can talk about
>> the coding style in more meaning full detail.
>> 
>> Keith
>> >
>> >Neil
>> >
>> >
>> 
>>