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? 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/? 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>
