Anonymous | Login | Signup for a new account | 2024-04-20 16:08 UTC |
My View | View Issues | Change Log | Roadmap | Doomseeker Issue Support Ranking | Rules | My Account |
View Issue Details [ Jump to Notes ] | [ Issue History ] [ Print ] | ||||||||
ID | Project | Category | View Status | Date Submitted | Last Update | ||||
0003591 | Doomseeker | [All Projects] Documentation | public | 2019-01-01 20:29 | 2019-07-30 10:13 | ||||
Reporter | Pol M | ||||||||
Assigned To | Pol M | ||||||||
Priority | normal | Severity | feature | Reproducibility | N/A | ||||
Status | closed | Resolution | fixed | ||||||
Platform | Linux | OS | Arch | OS Version | x86-64 | ||||
Product Version | 1.2 | ||||||||
Target Version | 1.3 | Fixed in Version | 1.3 | ||||||
Summary | 0003591: Programming style (for formatting tools like clang-format or astyle) is undocumented | ||||||||
Description | With 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 Reproduce | Look 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 Files | unintended-uncrustify.patch [^] (5,882 bytes) 2019-04-22 08:30 [Show Content] | ||||||||
Notes | |
(0020301) Pol M (developer) 2019-01-03 16:47 edited on: 2019-01-06 19:09 |
For instance, this astyle config:
meets quite closely the common style of doomseeker. |
(0020302) WubTheCaptain (reporter) 2019-01-06 06:25 |
Related to 0003526. We need a CONTRIBUTING file or a better mention in README. |
(0020303) WubTheCaptain (reporter) 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. 😊 |
(0020306) Pol M (developer) 2019-01-06 10:57 edited on: 2019-01-06 10:58 |
Quote from Wub 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 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 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:
Astyle changes the file once you save to:
Quote from Wub 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. |
(0020307) WubTheCaptain (reporter) 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.) |
(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.
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. |
(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 |
(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. |
(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 :) |
(0020529) Zalewa (developer) 2019-04-22 09:36 |
Quote from "Pol M" 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. |
(0020530) Pol M (developer) 2019-04-22 12:14 |
Mmm... the devil is in the details...Quote from Zalewa Welp, I guess that there's more reformatting in the way. I'm gessing you mean range-based for loops? Quote from Zalewa 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 Excluding some files should not be a problem. Quote from Zalewa I had seen it, and I once patched it to make it compile. Should not be a problem, I'll pr a fix asap. |
(0020531) WubTheCaptain (reporter) 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 That code is not from Turok 2 EX team, upstream rather. Anyway, it's a seperate issue. |
(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. |
(0020546) Pol M (developer) 2019-04-24 20:43 edited on: 2019-04-24 21:32 |
Got some tweaks: indent_shift will indent after stream processors, but please take note that some lines will end up with a double indentation caused by multiline strings in functions:
nl_brace_while set to "remove" will take care of do-while. About "intentional fall-through", there seems to not be a way to solve this. Maybe the best idea is to move it to previous line? Maybe I'll try to open a ticket, but for the moment this is what we got. |
(0020547) Zalewa (developer) 2019-04-24 21:55 |
The "intentional fall-through" problem can probably be fixed by wrapping the `case` in a curly braces block, but it doesn't look good to me when a curly braces block has an "intentional fall-through". Maybe converting this comment to a block comment will make the uncrustify tool treat it differently? Or, as you said, we could swap the lines. |
(0020643) Pol M (developer) 2019-05-13 10:55 edited on: 2019-05-13 11:05 |
So, after even more research, the issue with the AND operators seems to not have a solution yet. My best suggestion is to stop formatting "&" and "*", and agree to put them in the right when dealing with pointers and references. This is far from ideal, but I don't see any other solution. There is a ticket with the exact same issue as ours, once it gets resolved we could reconsider enforcing the formatting. I'd like to know if you're okay with this. About the indenting with <<, the issue can be worked around with pp_define_at_level set to false. And finally, the fallthrough should be solved by moving the comment wherever it is appropriate. I'd add the comment at the end of the last line of the case:
Making it a block comment doesn't change the behaviour, but wrapping it with curly braces does so. Regardless, the solution that looks the best to me is the line swap. |
(0020644) Zalewa (developer) 2019-05-13 15:32 edited on: 2019-05-13 16:00 |
"indent_shift = true" screws up enum braces in "src/plugins/zandronum/zandronumgameinfo.h". Oh well, I suppose it's a lesser evil than the incorrect stream indentation that happens when the setting is false. I have merged the outstanding changes in the PR: 1.'https://bitbucket.org/Doomseeker/doomseeker/commits/9f8b3b20bee73df6a60e231afeda165f250ca20b [^]' 2.'https://bitbucket.org/Doomseeker/doomseeker/commits/5ef265202a52c2e351c107c37dbcc6f6d4da636e [^]' 3.'https://bitbucket.org/Doomseeker/doomseeker/commits/02a4531f2f59f63919954c44acf76df1e99c2a0d [^]' And uncrustified: 4.'https://bitbucket.org/Doomseeker/doomseeker/commits/cbc0c0615d9b9bc43507a17cd278b20ba5673fb0 [^]' If you wish you may run the following script and notice that some of the files will still be changed:
There's the known & problem, but I'd rather keep it as is. It's much more beneficial to have the & moved to the right in all the cases where it needs to be moved rather than to save ourselves from these 4 cases where it is moved incorrectly. It appears that the combination of & and :: being in the same line actually screws this up as lines that don't have them and have the binary AND are formatted correctly. The ticket you linked to also shows an example with :: There's also a pointer * that is possibly interpreted as a multiply operator in engineplugin.h in a #define. And there are the incorrectly indented } in enums as mentioned before. If any of these problems can be fixed then it'd be good to fix them, but anyway I think the main goal of this ticket has been achieved |
(0020646) Pol M (developer) 2019-05-13 17:37 |
Quote from Zalewa a "," at the end of the enum fixes the issue :) PR Quote from Zalewa I'd also like to close this tiket for the moment. There are small errors, but I don't mind them so much. |
(0020647) Zalewa (developer) 2019-05-13 17:42 |
With the final patch merged I am closing this as resolved. As always, thanks for preparing the cfg file and all the other help. |
This issue is already marked as resolved. If you feel that is not the case, please reopen it and explain why. |
|
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 |
2019-04-24 20:43 | Pol M | Note Added: 0020546 | |
2019-04-24 21:32 | Pol M | Note Edited: 0020546 | View Revisions |
2019-04-24 21:55 | Zalewa | Note Added: 0020547 | |
2019-05-13 10:55 | Pol M | Note Added: 0020643 | |
2019-05-13 10:56 | Pol M | Note Edited: 0020643 | View Revisions |
2019-05-13 11:05 | Pol M | Note Edited: 0020643 | View Revisions |
2019-05-13 11:15 | Pol M | Status | assigned => needs review |
2019-05-13 15:32 | Zalewa | Note Added: 0020644 | |
2019-05-13 16:00 | Zalewa | Note Edited: 0020644 | View Revisions |
2019-05-13 17:37 | Pol M | Note Added: 0020646 | |
2019-05-13 17:42 | Zalewa | Note Added: 0020647 | |
2019-05-13 17:42 | Zalewa | Status | needs review => resolved |
2019-05-13 17:42 | Zalewa | Fixed in Version | => 1.3 |
2019-05-13 17:42 | Zalewa | Resolution | open => fixed |
2019-07-30 10:13 | WubTheCaptain | Status | resolved => closed |
Copyright © 2000 - 2024 MantisBT Team |