From mboxrd@z Thu Jan 1 00:00:00 1970 From: Neil Horman Subject: Re: tools brainstorming Date: Fri, 10 Apr 2015 07:41:19 -0400 Message-ID: <20150410114119.GA26772@hmsreliant.think-freely.org> References: <20150408131105.GD22959@hmsreliant.think-freely.org> <0C5AFCA4B3408848ADF2A3073F7D8CC86D58FDBF@IRSMSX109.ger.corp.intel.com> <0FBA33A7-A21E-426F-B44E-32E86F2B23DB@infiniteio.com> <20150408153802.2bc59227@urahara> <20150409191658.GC26201@hmsreliant.think-freely.org> <20150410002649.GB28597@neilslaptop.think-freely.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Cc: "dev-VfR2kkLFssw@public.gmane.org" To: "Wiles, Keith" Return-path: Content-Disposition: inline In-Reply-To: List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" On Fri, Apr 10, 2015 at 01:49:33AM +0000, Wiles, Keith wrote: >=20 >=20 > On 4/9/15, 7:26 PM, "Neil Horman" wrote: >=20 > >On Thu, Apr 09, 2015 at 09:10:19PM +0000, Wiles, Keith wrote: > >>=20 > >>=20 > >> On 4/9/15, 2:38 PM, "Jay Rolette" wrote: > >>=20 > >> >On Thu, Apr 9, 2015 at 2:16 PM, Neil Horman > >>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-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.org> wrote: > >> >> > > >> >> > > On Wed, 8 Apr 2015 16:29:54 -0600 > >> >> > > Jay Rolette 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 o= f > >>what > >> >> type of > >> >> comments should be allowed (unless there is a compelling reason t= o > >> >>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 fo= r > >> >things > >> >like this? > >> > > >> >Should we force someone to spend time redoing a patch because of wh= ich > >>way > >> >they do their parenthesis? What about number of spaces to indent co= de? > >>// > >> >vs /* */ comments? None of these matter functionally and they don't > >>affect > >> >maintenance generally. > >> > > >> >If someone is modifying existing code, then yeah, they should follo= w > >>the > >> >prevailing style (indention level, brace alignment, etc.) of the fi= le > >>they > >> >are in. It helps readability, which makes maintenance easier. Howev= er, > >> >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 h= ave > >> >used > >> >/* */ for comments, I'm extremely unlikely spend extra time to resu= bmit > >> >the > >> >patch for pedantry. > >>=20 > >> I looked at checkpatch.pl for few minutes and the code does check fo= r > >>C99 > >> comments and adding a command line option to allow C99 comments coul= d > >> 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 op= tion > >> was to not allow C99 comments. > >>=20 > >> 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 compiler= s we > >> support allow C99 comments. > >>=20 > >> The only other reason to allow them is if we add some open source co= de > >>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. > >>=20 > > > >So, this again seems to be bad philosophy in my mind. If we are, to u= se > >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. >=20 > 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. >=20 No one said anyone is violating our style guidelines, I think thats prett= y obvious. You had said that allowing C99 comments would be a nice thing t= o be able to do in the future if we were to want to integrate a submission tha= t 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 an= y specific set of rules we currently do or may in the future assert) are th= ere to enforce a consistent look in the code. Put another way, we can certainly allow for both styles of comment (or mu= ltiple styles of any aspect of the code), if we want to, but doing so is somewha= t against the notion of a 'style', as a style provides a consistent look, a= nd 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 mult= iple types of any kind of style aspect), only that, if we are to consider mult= iple 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 r= eview. 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 fai= rly spicific with very limited options for style choices If the goal is a happy medium, we should make sure that we have a tool th= at 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 th= e > >side of > >going with what the tool (checkpatch or something else) does, unless w= e > >have a > >maintainer stepping up. >=20 > Sure maintaining a tool for the C99 comment seems pretty easy, if no on= e > 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 mea= ns 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 po= int > 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 checkpat= ch > provides some checks we seem to agree with. >=20 You're right here, we should be deciding if we want to support C99 commen= ts 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 tha= t 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 tha= t 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 =C5=92our=C2= =B9 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. >=20 Yes, we do, we have to, otherwise style never gets enforced. Though asty= le 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 o= perate 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 guideline= s. > I was suggesting we could use one of these tool to create a common conf= ig > file for everyone to use and then checkpatch may not be required. At le= ast > these other tools beside check patch seem to have a huge number of opti= ons > 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 wron= g, 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 tha= t is > >willing to maintain such a tool, then we have a lot of leway in what t= he > >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. >=20 > For tools like astyle or uncrustify we do not have to maintain those to= ols > as they are already being maintained, we just have to use them. It seem= s a > waste to me we are not willing to change because someone may have to ma= ke > 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 formattin= g. >=20 I'm not opposed to making a change to checkpatch, we just need to know wh= os 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.=20 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. >=20 Question: What about the kernel code that we borrow from the upstream pro= jects (igb/e1000/ixgbe kernel code)? Are you proposing an automatic reformatti= ng there? If we have expectations of integrating future kernel code, it mig= ht be worthwhile adopting the kernel style (at least for those files). I reali= ze 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 w= e 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 i= t > will not change anytime soon. If you applied a tool to format all of th= e > code at some flag day, we would have a bit of churn then it would be ov= er. >=20 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 sty= le rather than automatically correcting them. > I would suggest we focus on the coding style and see what it means to t= he > tool/checkpatch. Read the email from Siobhan and then we can talk about > the coding style in more meaning full detail. >=20 > Keith > > > >Neil > > > > >=20 >=20