Zandronum Chat @ irc.zandronum.com
#zandronum
Get the latest version: 3.0
Source Code

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0003591Doomseeker[All Projects] Documentationpublic2019-01-01 20:292019-04-23 08:16
ReporterPol M 
Assigned ToPol M 
PrioritynormalSeverityfeatureReproducibilityN/A
StatusassignedResolutionopen 
PlatformLinuxOSArchOS Versionx86-64
Product Version1.2 
Target Version1.3Fixed in Version 
Summary0003591: Programming style (for formatting tools like clang-format or astyle) is undocumented
DescriptionWith to objective to ensure that a new user (and me) uses the official format for doomseeker, we could consider using one of these tools. Also, this will allow us to define some more precise rules in formatting without implying a lot of time wasted in following formatting rules (they can easily be integrated into most IDEs). This ticket, unfortunately, would require a commit updating files.
Steps To ReproduceLook at a pull request from a new user (me). Notice that there are formatting errors. Also, I've noticed some minor style differences in different files.
Additional Information
Attached Filespatch file icon unintended-uncrustify.patch [^] (5,882 bytes) 2019-04-22 08:30 [Show Content]

- Relationships
child of 0003595confirmedWubTheCaptain Contributor documentation is not yet available 

-  Notes
User avatar (0020301)
Pol M (developer)
2019-01-03 16:47
edited on: 2019-01-06 19:09

For instance, this astyle config:

mode=c
style=allman
indent=force-tab=4
indent-cases
indent-namespaces
indent-after-parens
indent-continuation=1
min-conditional-indent=1
max-continuation-indent=80
align-pointer=type
align-reference=type

pad-oper
pad-comma
pad-header

#attach-return-type
#attach-return-type-decl

#remove-braces

meets quite closely the common style of doomseeker.

User avatar (0020302)
WubTheCaptain (developer)
2019-01-06 06:25

Related to 0003526. We need a CONTRIBUTING file or a better mention in README.
User avatar (0020303)
WubTheCaptain (developer)
2019-01-06 06:55

Now, I'm not so sure how useful documenting these rules would be without continuous integration (CI) checks. Our CI is kind of "meh" currently?

Maybe an experienced programmer can look around the source code and try to mimic the existing programming style. Or maybe the maintainer decides to accept a patch, but further contribute to fit the programming style wanted right after.

If there are "some minor style differences in different files", it's an issue we should fix; please create a new issue ticket for this for me to work on. 😊
User avatar (0020306)
Pol M (developer)
2019-01-06 10:57
edited on: 2019-01-06 10:58

Quote from Wub

Now, I'm not so sure how useful documenting these rules would be without continuous integration (CI) checks.

Since these tools automatically enforce a formatting style, there is no need to change the current CI config. A developer simply has to integrate the desired tool in their editor of choice and then make a pull request. If the said developer does not use the formatting rules, Zalewa (for instance) can run the formatting rules and see that they are not followed (no need to check for whitespace, brackets, proper indentation...). (then he would inform the developer)
Quote from Wub

Our CI is kind of "meh" currently?

There was the idea of making a clone of the repo in git and pushing it to Github to use Travis, a really good CI tool (possible ticket). For the moment it does the minimum: ensuring that broken commits in the default branch (marked with a "@") don't go unnoticed. This is good enough, but of course, it can be a lot better
Quote from Wub

Maybe an experienced programmer can look around the source code and try to mimic the existing programming style. Or maybe the maintainer decides to accept a patch, but further contribute to fit the programming style wanted right after.

This is why a formatting tool is useful: no need to worry about style any more. for instance, let's say we agree that in if/else/for cases we want a space after those words and that brackets should be in separate lines. Then if I write this:

if(!plugin.data()->demoExtensionAutomatic){}

Astyle changes the file once you save to:

if (!plugin.data()->demoExtensionAutomatic)
{
}

Quote from Wub

If there are "some minor style differences in different files", it's an issue we should fix; please create a new issue ticket for this for me to work on.

Since we don't have any rules defined yet if I were you I'd wait until we achieve some kind of resolution in this ticket before creating another one. If at the end of this ticket we achieve a file with formatting rules for one of those tools, it would be as easy as running it in all the files (except the ones that are not ours, like the "dependencies folder" and other files inside the source tree) with a simple enough command.

User avatar (0020307)
WubTheCaptain (developer)
2019-01-08 14:27

For the note, I have no preference for C++ formatting either way. (For C, it'd be OpenBSD KNR or K&R/ANSI-C.)
User avatar (0020337)
Pol M (developer)
2019-02-05 18:17
edited on: 2019-02-07 14:48

I propose the following .clang-format style. I've tried to adapt what I've been told and what I feel to be good style.


DisableFormat: false
Language: Cpp
Standard: Cpp03
TabWidth: 4
IndentWidth: 4
UseTab: ForContinuationAndIndentation

AccessModifierOffset: -4
AlignAfterOpenBracket: DontAlign
AlignConsecutiveAssignments: false
AlignConsecutiveDeclarations: false
AlignEscapedNewlines: Left
AlignOperands: false
AlignTrailingComments: true
AllowAllParametersOfDeclarationOnNextLine: false
AllowShortBlocksOnASingleLine: true
AllowShortCaseLabelsOnASingleLine: false
AllowShortFunctionsOnASingleLine: All
AllowShortIfStatementsOnASingleLine: false
AllowShortLoopsOnASingleLine: false
AlwaysBreakAfterReturnType: None
AlwaysBreakBeforeMultilineStrings: false
AlwaysBreakTemplateDeclarations: No
BinPackArguments: true
BinPackParameters: true
BreakBeforeBraces: Custom
BraceWrapping:
  AfterClass: true
  AfterControlStatement: true
  AfterEnum: true
  AfterFunction: true
  AfterNamespace: true
  AfterStruct: true
  AfterUnion: true
  AfterExternBlock: true
  BeforeCatch: true
  BeforeElse: true
  SplitEmptyFunction: false
  SplitEmptyRecord: false
  SplitEmptyNamespace: false
BreakBeforeBinaryOperators: None
BreakConstructorInitializers: AfterColon
BreakInheritanceList: AfterColon
BreakStringLiterals: false
ColumnLimit: 80
CompactNamespaces: false
ConstructorInitializerAllOnOneLineOrOnePerLine: false
ConstructorInitializerIndentWidth: 4
ContinuationIndentWidth: 4
Cpp11BracedListStyle: false
DerivePointerAlignment: true
FixNamespaceComments: false
ForEachMacros: ['foreach']
IncludeBlocks: Preserve
IndentCaseLabels: true
IndentPPDirectives: None
IndentWrappedFunctionNames: true
KeepEmptyLinesAtTheStartOfBlocks: false
MaxEmptyLinesToKeep: 1
NamespaceIndentation: All
PointerAlignment: Left
ReflowComments: true
SortIncludes: true
SortUsingDeclarations: true
SpaceAfterCStyleCast: true
SpaceAfterTemplateKeyword: false
SpaceBeforeAssignmentOperators: true
SpaceBeforeCtorInitializerColon: true
SpaceBeforeInheritanceColon: true
SpaceBeforeParens: ControlStatements
SpaceInEmptyParentheses: false
SpacesBeforeTrailingComments: 1
SpacesInAngles: false
SpacesInCStyleCastParentheses: false
SpacesInParentheses: false
SpacesInSquareBrackets: false

PenaltyBreakAssignment: 30
PenaltyBreakComment: 5
PenaltyBreakFirstLessLess: 30
PenaltyBreakString: 25
PenaltyBreakTemplateDeclaration: 100
PenaltyExcessCharacter: 15
PenaltyReturnTypeOnItsOwnLine: 100

There are a few minor things that I do not like. For instance, clang will move to the left preprocessor directives, and I'd like that class declarations had an extra indent, but unfortunately, it is not possible without messing up other things.
EDIT: Also, it tries to align chained function calls (Ex: object.function1().function2();) when they are in different lines instead of indenting, and more of the same with multiline strings. I'll try to solve this because this annoys me more.

User avatar (0020360)
Pol M (developer)
2019-02-10 20:00

I've decided to give a chance to uncrustify. I originally did not want to use it because of the number of parameters it used (655) and because it's less well known, but after (lots) of tweaking, I believe to have a good config. Most of it is set to "ignore", so it is quite permissive. So far, I could not find something I did not like. pr
User avatar (0020521)
Zalewa (developer)
2019-04-21 13:39

I've added some personal tweaks to the cfg file and I think it works quite well for header files now. I pushed the uncrustified headers here. I also have many of the .cpp files ready but at the same time I still have many more to process. In their case the cfg file still doesn't work exactly as I'd like it to. I'll process the remaining .cpp files first (and possibly push them) and then see what can be done about the stuff that remains.

I'll merge the PR after that.
User avatar (0020525)
Pol M (developer)
2019-04-21 16:24

Looking pretty good. What issues do you have with .cpp files? The last days of exams are coming soon here, but I'd love to help in my free seconds :)
User avatar (0020529)
Zalewa (developer)
2019-04-22 09:36

Quote from "Pol M"
What issues do you have with .cpp files?


See "unintended-uncrustify.patch". The '-' lines show the correct formatting, the '+' lines are the result of uncrustify. To sum it up:

- Bad indentation when the stream operator is involved.
- Bad indentation of 'intentional fall-through' comment in 'switch-case'.
- "do-while" - the "while" keyword should be on the same line as the closing brace.
- The binary AND (&) operator seems to be considered a reference operator and is not surrounded by spaces correctly in the 'if' statements.

Also the pointer '*' in Qt's foreach macros is possibly recognized as a multiplication operator as it is being surrounded by spaces from both sides. I actually had to search & replace those when reviewing the files to avoid committing them. However, this can be solved by converting the 'foreach' macros into C++11's for-each loops. Also, the foreach macro is deprecated since Qt 5.7.


The work that we have so far is:

- Uncrustify .h files
- Uncrustify .cpp files
- Merge the PR with uncrustify.cfg
- My own tweaks to uncrustify.cfg

In the middle of going through the files I decided that having extraneous {} for a single-line conditionals is okay and switched the relevant uncrustify setting to ignore, but there are .cpp files committed where the {} are removed. As far as I'm concerned this is fine.


Aside from the minor formatting issues mentioned above, there are also organisational issues to resolve:

1. What would be the proposed approach to ensure that this is applied to all commits? For me, at least, it appears that Emacs can integrate with uncrustify. I haven't checked that yet but if it works as expected then it should be enough. What about everyone else?

2. As with all automatic tools, sometimes (rarely, but still) the tool is wrong and it should be told to back off. We already have several examples of this in our repo when it comes to "external" code:

- json, huffman and lzma code should not be touched. It should be easy enough to deal with this by filtering out whole files.
- dmflags in Zandronum plugin are ripped straight out from Zandronum source. These should not be touched for easier diffing with the original code in Zandronum, however all of the code that surrounds them is free to be uncrustified.
- Turok 2 EX CRC code refuses to compile after uncrustification due to reliance on order of #includes, however this can be considered as bug and fixed.
User avatar (0020530)
Pol M (developer)
2019-04-22 12:14

Mmm... the devil is in the details...
Quote from Zalewa

This can be solved by converting the 'foreach' macros into C++11's for-each loops.

Welp, I guess that there's more reformatting in the way. I'm gessing you mean range-based for loops?
Quote from Zalewa

1. What would be the proposed approach to ensure that this is applied to all commits? For me, at least, it appears that Emacs can integrate with uncrustify. I haven't checked that yet but if it works as expected then it should be enough. What about everyone else?

There are extensions/ways to integrate it into VSCode, VS, Sublime Text, atom, and there seems to be a way to use it in Vim (though, if you are capable of exiting it, I'm sure you'll figure out a way). I don't think that integration will be an issue, so I'd expect commits to use it. That said, if the author forgot to use uncrustify in a file, a comment explaining what Uncrustify is should be enough (the same way I was told to use tabs instead of spaces).

Quote from Zalewa

- dmflags in Zandronum plugin are ripped straight out from Zandronum source. These should not be touched for easier diffing with the original code in Zandronum, however all of the code that surrounds them is free to be uncrustified.

Excluding some files should not be a problem.
Quote from Zalewa

- Turok 2 EX CRC code refuses to compile after uncrustification due to reliance on order of #includes, however this can be considered as bug and fixed.

I had seen it, and I once patched it to make it compile. Should not be a problem, I'll pr a fix asap.
User avatar (0020531)
WubTheCaptain (developer)
2019-04-22 18:37

I think this ticket is done, the config files are in. I wouldn't want to keep this ticket open much longer, the core issue seems to be resolved. Anyone agree to close this as "fixed in 1.3" and "resolved"?

Quote from Zalewa
Turok 2 EX CRC code refuses to compile after uncrustification due to reliance on order of #includes, however this can be considered as bug and fixed.


That code is not from Turok 2 EX team, upstream rather. Anyway, it's a seperate issue.
User avatar (0020532)
Zalewa (developer)
2019-04-23 08:16

It is not resolved yet as the attached "unintended-uncrustify.patch" shows that there still are undesirables that should not happen. The configuration should be tweaked so that these stop happening or the code needs to be mangled in order to make it fit with the configuration.

Issue Community Support
Only registered users can voice their support. Click here to register, or here to log in.
Supporters: Pol M WubTheCaptain
Opponents: No one explicitly opposes this issue yet.

- Issue History
Date Modified Username Field Change
2019-01-01 20:29 Pol M New Issue
2019-01-01 20:29 Pol M Status new => assigned
2019-01-01 20:29 Pol M Assigned To => Pol M
2019-01-01 20:29 Pol M Status assigned => feedback
2019-01-03 16:47 Pol M Note Added: 0020301
2019-01-03 16:47 Pol M Status feedback => assigned
2019-01-03 16:47 Pol M Assigned To Pol M =>
2019-01-03 16:47 Pol M Status assigned => feedback
2019-01-06 06:25 WubTheCaptain Note Added: 0020302
2019-01-06 06:25 WubTheCaptain Assigned To => WubTheCaptain
2019-01-06 06:25 WubTheCaptain Status feedback => confirmed
2019-01-06 06:25 WubTheCaptain Relationship added related to 0003526
2019-01-06 06:28 WubTheCaptain Status confirmed => acknowledged
2019-01-06 06:31 WubTheCaptain Category Cleanup => Documentation
2019-01-06 06:40 WubTheCaptain Summary Define an official format using a formatting tool like clang-format or astyle => Programming style (for formatting tools like clang-format or astyle) is undocumented
2019-01-06 06:43 WubTheCaptain Additional Information Updated View Revisions
2019-01-06 06:44 WubTheCaptain Additional Information Updated View Revisions
2019-01-06 06:45 WubTheCaptain Additional Information Updated View Revisions
2019-01-06 06:55 WubTheCaptain Note Added: 0020303
2019-01-06 06:59 WubTheCaptain Relationship deleted related to 0003526
2019-01-06 07:04 WubTheCaptain Relationship added child of 0003594
2019-01-06 07:26 WubTheCaptain Relationship added parent of 0003595
2019-01-06 07:27 WubTheCaptain Relationship deleted child of 0003594
2019-01-06 07:27 WubTheCaptain Relationship replaced child of 0003595
2019-01-06 07:31 WubTheCaptain Product Version => 1.2
2019-01-06 10:57 Pol M Note Added: 0020306
2019-01-06 10:58 Pol M Note Edited: 0020306 View Revisions
2019-01-06 19:09 Pol M Note Edited: 0020301 View Revisions
2019-01-08 14:27 WubTheCaptain Note Added: 0020307
2019-02-05 18:17 Pol M Note Added: 0020337
2019-02-05 18:21 Pol M Note Edited: 0020337 View Revisions
2019-02-07 14:48 Pol M Note Edited: 0020337 View Revisions
2019-02-10 20:00 Pol M Note Added: 0020360
2019-02-11 16:09 WubTheCaptain Assigned To WubTheCaptain => Zalewa
2019-02-11 16:09 WubTheCaptain Status acknowledged => needs review
2019-04-21 13:39 Zalewa Note Added: 0020521
2019-04-21 16:24 Pol M Note Added: 0020525
2019-04-22 08:30 Zalewa File Added: unintended-uncrustify.patch
2019-04-22 09:36 Zalewa Note Added: 0020529
2019-04-22 12:14 Pol M Note Added: 0020530
2019-04-22 18:05 Zalewa Assigned To Zalewa => Pol M
2019-04-22 18:05 Zalewa Status needs review => assigned
2019-04-22 18:37 WubTheCaptain Note Added: 0020531
2019-04-22 18:37 WubTheCaptain Priority none => normal
2019-04-22 18:37 WubTheCaptain Status assigned => needs testing
2019-04-22 18:37 WubTheCaptain Target Version => 1.3
2019-04-23 08:16 Zalewa Note Added: 0020532
2019-04-23 08:16 Zalewa Status needs testing => assigned






Questions or other issues? Contact Us.

Links


Copyright © 2000 - 2019 MantisBT Team
Powered by Mantis Bugtracker