Hi Adam,
See inline… *From:* Adam Feuer [mailto:a...@starcat.io] *Sent:* Tuesday, March 17, 2020 12:10 PM *To:* David Sidrane *Cc:* dev@nuttx.apache.org *Subject:* Re: Nuttx Code Formatter Progress [Was RE: Should we relax precheck a little bit?] David, Yes, I agree that it would be great if we can upstream the changes. I think that's possible, and I think we should try before making the decision to maintain our own fork. Yes, I also agree that the current config fixes a bunch of things, but also messes up a lot of other things too. Re: # indenting. I am not sure I understand your comments about this. Are the NuttX Style Guide sections on indenting <https://cwiki.apache.org/confluence/display/NUTTX/Coding+Standard#indentation> correct? If so, we can probably add clang-format options to support this style. If not, can we correct the Style Guide? [DBS] The correct thing to do is indent on nested #, but we have to ignore the #if defined (THIS FILE) So the next # is at Col1. This is the convention for thew old way of doing #pragma once #if !defined(this file) #define this file #define FOO #endif /* !defined(this file) */ This is what you would infer from the Style Guide, but now looking at it I see it has been updated #if !defined(this file) # define this file # define FOO #endif /* !defined(this file) */ Re: clang-format having a greater ROI, I think that's true if we can get it to work. It has a big community supporting it. I will create a PR for the .clang-format and a README, and mark it [Do Not Merge] and DRAFT PR. Do you also want me to include the formatted files under sched/? [DBS] Yep all of it. So we can review it and play with it all. cheers adam On Tue, Mar 17, 2020 at 5:56 AM David Sidrane <david.sidr...@nscdg.com> wrote: Adam, Thank you for putting in the effort on this! I only suggest forking incase the LLVM project finds Nuttx' CS not of value. It would be great if we can upstream the changes. If we make this a DNM (Do Not Merge) Pull request we can discuss in-line where the comments will have context. Just put [Do Not Merge] in the title and Leave it set at DRAFT PR. Hopefully everyone will understand that we are doing this to collaborate and add valuable feedback. My overall view is it fixed a bunch of stuff NxStyle does not even detect. But it messes up a bunch of things too. One issue is the # indenting. Since Nuttx does not allow the modern use of `#pragma once` the guard #if defs set the indent to 1 to start with. I remember getting lambasted for doing this after reading the CS and following it. So we will need a -nuttx-option to set the indent to -1 and then only indent for > 0. Let's continue to take it apart in the PR and see what the level of effort would be to get it in shape. Personally, I am not afflicted with NIH syndrome, I value others quality tools, and I would rather put the effort in to this mature tool than Nxstyle, as the ROI will be much, much greater. David *From:* Adam Feuer [mailto:a...@starcat.io] *Sent:* Monday, March 16, 2020 3:35 PM *To:* David Sidrane; dev@nuttx.apache.org *Subject:* Re: Nuttx Code Formatter Progress [Was RE: Should we relax precheck a little bit?] Here's a Github compare with .clang-format file and the results when clang-format-9 is run on all the files under sched/: https://github.com/apache/incubator-nuttx/compare/master...starcat-io:explore/clang-format-sched If anyone has comments or observations I would love to know them. -adam On Mon, Mar 16, 2020 at 3:23 PM Adam Feuer <a...@starcat.io> wrote: David, It would be a --style=NuttX option on the command line, and a set of configurations. But yes that might be the way to go. I don't think forking it would be the right thing, if we can modify it to do what we want, we should be able to add options that are configurable, submit PRs and get them merged. Then the features can be used and maintained by everyone who uses clang-format, a very large userbase, that would be a very good option. I think it can work... I'll post link to what it can currently do in sched/ so we can look at the differences to see how close we are. -adam On Sun, Mar 15, 2020 at 6:49 AM David Sidrane <david.sidr...@nscdg.com> wrote: Hi Adam, Hmmm…Would the shortest path, on this issue, be to add some -NuttX options and submit a PR to upstream or Fork it? It may be that time spent training Clang-format is better than time spent on nstyle, as it is a "shorter haul" with a much, much greater return. David *From:* Adam Feuer [mailto:a...@starcat.io] *Sent:* Saturday, March 14, 2020 1:59 PM *To:* dev@nuttx.apache.org; david.sidr...@nscdg.com; w8j...@gmail.com *Subject:* Re: Nuttx Code Formatter Progress [Was RE: Should we relax precheck a little bit?] I looked at the clang-format source code. It has trouble aligning the = of a -= or +=. I easily made a change to align on the - or + of a -= or +=, but some work will be necessary to give an option that aligns the way the nuttx code does it. Will think more about it. -adam On Sat, Mar 14, 2020 at 1:21 PM Adam Feuer <a...@starcat.io> wrote: David, Maciej, Peter, Thanks for your help! IndentPPDirectives: PPDIS_AfterHash works. The actual syntax for the .clang-format file is this: IndentPPDirectives: AfterHash That makes things a lot better. There are a bunch of inconsistent macro indents under sched/ though— many macros indent ok, but there are a bunch that don't. Not sure what to do about that... are they really inconsistent? If so maybe we make a small PR that fixes the inconsistent indents? What seems to be next: · alignment of successive expressions reltime.tv_nsec += NSEC_PER_SEC; - reltime.tv_sec -= 1; + reltime.tv_sec -= 1; · alignment of comment blocks — to make sure they line up with the next comment line in the block For instance: - /* The resulting set is the intersection of the current set and + /* The resulting set is the intersection of the current set and * the complement of the signal set pointed to by _set. */ · evaluating inconsistencies in the alignment style... some expressions and declarations are aligned, others are not... I need to consult the style guide to see what it says. I'm using clang-format-9. Here's the command lines I'm running to generate and look at the changes (in the nutt/ dir): $ find ./sched/ -iname "*.h" -or -iname "*.c" | xargs clang-format-9 -i -style=file $ git diff $ # change .clang-format $ git stash; git stash drop <repeat> Here's my .clang format file as of now: --- Language: Cpp AccessModifierOffset: -2 AlignAfterOpenBracket: Align AlignConsecutiveMacros: true AlignConsecutiveAssignments: true AlignConsecutiveDeclarations: true AlignEscapedNewlines: DontAlign AlignOperands: true AlignTrailingComments: true AllowAllArgumentsOnNextLine: true AllowAllConstructorInitializersOnNextLine: true AllowAllParametersOfDeclarationOnNextLine: true AllowShortBlocksOnASingleLine: false AllowShortCaseLabelsOnASingleLine: false AllowShortFunctionsOnASingleLine: All AllowShortLambdasOnASingleLine: All AllowShortIfStatementsOnASingleLine: Never AllowShortLoopsOnASingleLine: false AlwaysBreakAfterDefinitionReturnType: None AlwaysBreakAfterReturnType: None AlwaysBreakBeforeMultilineStrings: false AlwaysBreakTemplateDeclarations: MultiLine BinPackArguments: true BinPackParameters: true BraceWrapping: AfterCaseLabel: false AfterClass: false AfterControlStatement: true AfterEnum: true AfterFunction: true AfterNamespace: false AfterObjCDeclaration: false AfterStruct: true AfterUnion: true AfterExternBlock: false BeforeCatch: false BeforeElse: true IndentBraces: true SplitEmptyFunction: true SplitEmptyRecord: true SplitEmptyNamespace: true BreakBeforeBinaryOperators: None BreakBeforeBraces: Custom BreakBeforeInheritanceComma: false BreakInheritanceList: BeforeColon BreakBeforeTernaryOperators: true BreakConstructorInitializersBeforeComma: false BreakConstructorInitializers: BeforeColon BreakAfterJavaFieldAnnotations: false BreakStringLiterals: true ColumnLimit: 0 CommentPragmas: '^ IWYU pragma:' CompactNamespaces: false ConstructorInitializerAllOnOneLineOrOnePerLine: false ConstructorInitializerIndentWidth: 4 ContinuationIndentWidth: 0 Cpp11BracedListStyle: false DerivePointerAlignment: false DisableFormat: false ExperimentalAutoDetectBinPacking: false FixNamespaceComments: false ForEachMacros: - foreach - Q_FOREACH - BOOST_FOREACH IncludeBlocks: Preserve IncludeCategories: - Regex: '^"(llvm|llvm-c|clang|clang-c)/' Priority: 2 - Regex: '^(<|"(gtest|gmock|isl|json)/)' Priority: 3 - Regex: '.*' Priority: 1 IncludeIsMainRegex: '(Test)?$' IndentCaseLabels: true IndentPPDirectives: AfterHash IndentWidth: 2 IndentWrappedFunctionNames: false JavaScriptQuotes: Leave JavaScriptWrapImports: true KeepEmptyLinesAtTheStartOfBlocks: true MacroBlockBegin: '' MacroBlockEnd: '' MaxEmptyLinesToKeep: 1 NamespaceIndentation: None ObjCBinPackProtocolList: Auto ObjCBlockIndentWidth: 2 ObjCSpaceAfterProperty: false ObjCSpaceBeforeProtocolList: true PenaltyBreakAssignment: 2 PenaltyBreakBeforeFirstCallParameter: 19 PenaltyBreakComment: 300 PenaltyBreakFirstLessLess: 120 PenaltyBreakString: 1000 PenaltyBreakTemplateDeclaration: 10 PenaltyExcessCharacter: 1000000 PenaltyReturnTypeOnItsOwnLine: 60 PointerAlignment: Right ReflowComments: false SortIncludes: false SortUsingDeclarations: true SpaceAfterCStyleCast: false SpaceAfterLogicalNot: false SpaceAfterTemplateKeyword: true SpaceBeforeAssignmentOperators: true SpaceBeforeCpp11BracedList: false SpaceBeforeCtorInitializerColon: true SpaceBeforeInheritanceColon: true SpaceBeforeParens: ControlStatements SpaceBeforeRangeBasedForLoopColon: true SpaceInEmptyParentheses: false SpacesBeforeTrailingComments: 1 SpacesInAngles: false SpacesInContainerLiterals: true SpacesInCStyleCastParentheses: false SpacesInParentheses: false SpacesInSquareBrackets: false Standard: Cpp03 StatementMacros: - Q_UNUSED - QT_REQUIRE_VERSION TabWidth: 8 UseTab: Never ... On Sat, Mar 14, 2020 at 1:29 AM David Sidrane <david.sidr...@nscdg.com> wrote: Hi Adam and Maciej, Thank you for spending you time on this. It will be a huge win for everyone! The way I have been looking at this is like in transfer-function with an offset. Once we can get a tools that will take X and make into X" that has well known malformations. We just fix the malformations. So once you feel have something that is close, let's evaluate it and see what the "last mile" looks like. David -----Original Message----- From: Adam Feuer [mailto:a...@starcat.io] Sent: Friday, March 13, 2020 7:08 PM To: dev@nuttx.apache.org Subject: Re: Should we relax precheck a little bit? Maciej, Thank you! I didn't know about the IndentPPDirectives option! I will try it! :) -adam On Fri, Mar 13, 2020 at 5:16 PM Maciej Wójcik <w8j...@gmail.com> wrote: > Are you sure that clang-format cannot indent macros? What about > > IndentPPDirectives: PPDIS_AfterHash > > It also treats the outmost macro in headers in a special way. > > On Sat, 14 Mar 2020, 01:03 Adam Feuer, <a...@starcat.io> wrote: > > > David, > > > > Re: whatstyle, I ran it overnight on the c files in sched/ and came up > with > > a clang-format that does somewhat ok. Thanks for pointing that program > out. > > > > By looking at the output of the diff, I learned a lot about how hard it > is > > to manually format programs. :) > > > > Anyway, the biggest problem with clang-format seems to be the way it > > handles C-macros. In NuttX, they are often indented like this: > > > > #ifdef ... > > # define ... > > # ifdef ... > > # define > > # endif > > #endif > > > > Peter Van Der Perk also mentioned this. There's no stock way to make > > clang-format do that. Maybe a post-processing script that only looked at > > these macros would work. Or a contribution to clang-format. I'll think > > about it some more. > > > > -adam > > > > On Sun, Mar 8, 2020 at 3:40 AM David Sidrane <david.sidr...@nscdg.com> > > wrote: > > > > > Hi Adam, > > > > > > Have a look at https://github.com/mikr/whatstyle > > > > > > I got furthest with clang-format and it. It may be we get a 95% of the > > way > > > there with it and we can add a backend secondary scripts. > > > > > > I was unable to convince Greg to create a master template so my > approach > > > was > > > to combine all the files and run it on the set so it would get all the > > > constructs at once. > > > > > > David > > > > > > > -- > > Adam Feuer <a...@starcat.io> > > > -- Adam Feuer <a...@starcat.io> -- Adam Feuer <a...@starcat.io> -- Adam Feuer <a...@starcat.io> -- Adam Feuer <a...@starcat.io> -- Adam Feuer <a...@starcat.io> -- Adam Feuer <a...@starcat.io>
