-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mixed tabs and spaces #391
Comments
If you have the time to reformat all files according to NASA's C style guide then you could submit a pull request and I'll merge it. |
@Avamander, do you know some kind of source formatter (beautifier) software that would be able to format the code according to NASA C style? I have searched the Internet but I wasn't able to find such a software. I could find the only the docs related to the style: During my standard Arduino development I'm using Eclipse CDT with Sloeber Plugin. Eclipse CDT is able itself to format the code, by default only according to the following rules:
@Avamander, could you please write shortly why you prefer NASA C Style? Or what are pros for using NASA? I know that the code style treats about the "visual view of the code" (like the number of spaces in the indentation, the braces, where to put a new line and so on) but also it gives some clues about how to name correctly the functions, variables, and so on. |
@wmarkow It's both well written down, used in mission-critical places (thus one can say it's field-tested to be at least okay) and it most closely resembles what people write and what's used currently in the library. If you have better suggestions, feel free to comment them, and yeah, C++ style guide probably suits us more (and you probably can switch your IDE to Allman style as it's really close). |
I am not even a C programmer, but I could assist in just replacing tabs by 4 spaces in all .h and .c files. That would increase readability on Github via a Web browser as well as editors that render tabs with a number of spaces different from 4. Let me know. |
Thanks @MAKOMO but we can make the process automatic. I'm using Eclipse CDT for Arduino development. Eclipse has embedded code formatter. It is possible to create your own formatting rules, so I have created a formatter according to this NASA C++ guide. I have formatted two files with this formatter. There are commited in my RF24 fork in my fix_for_#391 branch. @Avamander, could you please take a look at my branch and make a short review? Thank you for your comment related to coding styles. I took a look and indeed you were right; Allman style is quite close to NASA. When I started to learn C/C++ many years ago I was taught some good coding styles but I didn't know that it was according to NASA. I like NASA coding style :) One more thing: if we decide to use this code formatter then it is better to take a look at the pull requests first and merge them before we format the code (we will avoid many merge conflicts that will be an effect of code formatting). And we could fix manually the #385, I mean to put the braces into one line if statements. |
@wmarkow: please go forward with this. 3 or 4 spaces are ok for me, just tabs should be avoided. I also agree that it might be easier to process pull requests first. The same work should also be applied to the Networking and Mesh lib. |
@wmarkow Your branch looks okay. Though the Arduino examples should be formatted with the Arduino IDE, should just be kept in mind when that bridge is to be crossed. Pull requests can usually be updated by other people (thus formatted too), if not opening a new one with the good style (without conflicts) would be totally okay imho. Another thing that should be done is a short description of the formatting style in the CONTRIBUTIONS.md and PULL_REQUEST_TEMPLATE file just to let people know it's established how to contribute code. |
I took a look at Arduino IDE 1.8.5 and how its formatting works. To auto format the code in Arduino IDE just hit Ctrl + T (or choose Tools->Auto Format). This autoformat works strange. It looks like it doesn't care about "brackets". This piece of code is formatted "correctly" with Arduino IDE:
I noticed that mostly it takes care about 2 spaces indentation and it put some spaces where needed. Now I'm just thinking that the Built-In Examples in Arduino IDE are the clue on what is their favorite code style (formatting). |
@wmarkow So you think we should format everything with the Arduino IDE? |
No, I think we can stick to the NASA format as you suggested before.
I just wanted to check what is the default formatting style in Arduino IDE but it looks like it allows some mixed formatting styles and actually you need manually take care of putting the brackets in a proper format. |
I think wmakow has the right idea with the example sketches: Follow the Arduino IDE's Tools > Auto Format conventions and then use the official Built-in Examples (and code from the Language Reference pages or other official code where that doesn't suffice) as a reference for any formatting choices not set by Auto Format. This is the style that Arduino users are comfortable with seeing in sketches. The Arduino IDE uses AStyle for the Auto Format. The stock configuration is here: AStyle also allows you to configure more formatting options, as documented here: I believe that adding the following options to the Arduino IDE's current AStyle configuration file will fully enforce the Arduino formatting style:
Note that some of these AStyle configuration options require a newer version of AStyle than is used by the Arduino IDE. The version of AStyle used by the Arduino IDE does not permit formatting to Arduino style braces because Even though it's in the stock Auto Format configuration, I would suggest removing the |
@per1234, thank you for your input about AStyle. I did some quick test on how Arduino IDE formats the code and I have found this a bit strange; take a look on one of my comments before. I have noticed like Arduino IDE doesn't force a specific brackets formatting. I had the feeling that this:
is correctly formatted, and also this is correctly formatted:
Maybe it is just a matter of AStyle configuration file (just like you pointed)? I'm not so familiar with AStyle. It could be worth to check. Because the issue I have noted above and because I'm also a Java programmer (and I'm using Eclipse), I started to use Eclipse formatter, because that was (for me) the fastest way to start experimenting with formatting. But enyway, I think I will take a look again at AStyle formatter. I think the best would be to have:
Going back to my exepriments with Eclipse formatter. |
@wmarkow The changes seem okay and really seem to remove quite a bit of uglyness, but I'm not sure about the static const char * const rf24_crclength_e_str_P[] PROGMEM = { rf24_crclength_e_str_0, rf24_crclength_e_str_1, rf24_crclength_e_str_2,}; vs. old: static const char * const rf24_crclength_e_str_P[] PROGMEM = {
rf24_crclength_e_str_0,
rf24_crclength_e_str_1,
rf24_crclength_e_str_2,
}; What do you think? |
You're correct that the stock AStyle configuration doesn't enforce a brackets style. As established by the majority of official example sketches, the Arduino brackets style is definitely attached, which is why I suggested the addition of I had thought the closing bracket and else statement were generally put on separate lines ( I think the formatting script is a great idea. I would even suggest that a formatting style compliance check be done on all commits and pull requests via an online continuous integration service (e.g. Travis CI, CircleCI). AStyle is well suited for that usage because it's a very lightweight command line application but other options may be just as good. |
I did some further investigations with Eclipse formatter and I have found something, what I do not like. It seems that formatter behaves a bit different when I run in outside of Eclipse (by calling some formatter classes from Eclipse core jar) with comparing to how it behave when I format the code from the Eclipse. In the meantime I took a look at AStyle formatter. I did some preliminary work and I must say that I'm surprised with this tool. It is lightweight, has a lot of configuration switches and can be run from command line easily. I think I will continue with testing AStyle. I will create a separate branch in my RF24 fork to test AStyle. I will try to make some "small" commits so we could see the differences in small steps. @Avamander, related to your question. Was it in opposite way?
vs new:
It could be that I did it by mistake. @per1234, thanks for your input related to AStyle and your configuration hints. I will try them and let you know later. |
@wmarkow Yeah, mixed up the words. I think the expanded version is more readable, what do you think? |
Yes, I think expanded version is more readable, especially when you have a few items in the table. Formatter will break the line anyway if the line will be longer than maximal characters per line; then everything will be mixed up. With expanded version you have everything in order: opening bracket, items (one item per line), closing bracket. I will try to prepare the AStyle config so it will put it expanded and we will see how it looks. |
BTW I do prefer the readthedocs-sphinx theme over the doxy-gen output that http://tmrh20.github.io/RF24 uses. There is a sphinx extension that will wrap doxy-gen docs, but I haven't tried it out... yet. I'm getting off topic again; that should be a separate issue when I start understanding how to use doxy-gen. |
Doxygen could be configured to use m.css that looks like this. But yes, this is a separate topic. |
I just did my tests with formatters like 2 years ago and that's it - from my side.
I think I lost the scope during those 2 years. What kind of formatter we use currently? Is it NASA code style? Personally I like NASA.
This may be done in purpose. 8 space indentation plus for example 80 characters per line means that you can't write the code with many nested if or loops statements because it will not fit on your terminal screen. Besides many nested code looks bad and is hard to understand and analyse. |
@per1234 I tried the job in this workflow, using it as an attempt to try and prevent the long-running "build" job that "needs" it to pass. It found a lot wrong with the examples (may have been my fault), so I went through all the examples the workflow marked as erroneous. After committing the changes from auto-formatter tool in the Arduino IDE, the workflow still marked all the same files as erroneous. You can see the results here (if you're interested), but be warned its a 2000+ line log. Just thought I'd let you know (I think I'll go without the format checking job). |
Here's the clue for why this happened:
You didn't add the formatter configuration file: You can see here the passing check in my fork: After I added https://github.com/arduino/arduino-examples/blob/main/examples_formatter.conf, there was still quite a bit of non-compliant formatting. The reason is that the formatter configuration provided in the arduino/arduino-examples repo adds some additional configuration options beyond that of the configuration used by the Arduino IDE's Auto Format. This means that, though code formatted with the "examples_formatter.conf" configuration will always be compliant with Arduino IDE's auto format configuration, the reverse is not always true. My quick fix for the sake of a proof of concept was to just replace the configuration with the one used by the Arduino IDE:
You can get astyle compiled for Windows here: https://sourceforge.net/projects/astyle/files/ After changing the configuration to match the IDE's auto formatter, there were still a couple examples that weren't compliant. Maybe you just missed those by accident. After that, there was one last recalcitrant file:
This With that, the check now passes: Since the time I made that comment about the The functionality is the same, but I have moved that ugly bash code into a GitHub Actions action. This makes it so the workflow can be very minimal and friendly to configure and the check code itself can be maintained from a single centralized location: |
As always, thanks for very detailed & extremely helpful reply. I think I will have another go at the format checking job. Although I'm in the process of basically replacing all examples with a library-relevant few. I find having a format checking job is good practice to avoid compiling malformed code on ALL supported boards (the matrix list is going to be huge). |
@per1234 I'm loving the artistic-style-action. Ran into yet another yml snag: list of file formats requires a single quote I think I'm ready to call this issue resolved. |
I'm very happy if it can be useful! I actually haven't used it on any other CI projects since the megaTinyCore repo, but it was a fun opportunity to learn about GitHub's new "composite run steps" action type.
YAML certainly has some tricky things like that. The language seems so simple, right up until it isn't! I tend to always quote everything, with the most strict quotes possible, but it seems the common practice with YAML is just the opposite (which I suppose does work out well for the most basic use cases).
That's not so bad. You're still pretty close to the standard Arduino formatting style. that |
assigned myself this issue (as a reminder) because it will likely be closed after merging #691 |
So, examples are properly formatted to AStyle preferences (configured to suite the Arduino IDE) as well as any source code. I rewrote the Linux examples with NASA C style guide in mind. All python examples follow PEP8 standards. Some doxygen comment blocks seemed misaligned or not conformed with RF24.h comment block style, and there may still be some more like that. However, #691 changed 104 files with this issue in mind, so I'm going to call this resolved until otherwise noted. If there are any other formatting errors that may have been missed, we can re-open this or do a quick PR/commit. |
@Avamander I've been looking at clang-format as a tool (also available as GH actions) we can use to check C/C++source code format. It comes with some builtin/popular style-guides that can be specified as what rules to follow, but there is an additional option to make our own set of rules (which can inherit from the aforementioned style guides) that gets saved to a file in the root of the repository. More info can be found here. BTW I really like the Webkit style guide, but there are some rules I'd like to ignore/override. Still, I don't think we can beat the practicality and flexibility of the NASA C style guide (the link in CONTRIBUTING.md works again - they must have finished their site update). |
Arduino decided to switch from Astyle to ClangFormat for Arduino IDE 2.x. There is also some work to do the same for the classic Arduino IDE: arduino/Arduino#11543 The ClangFormat configuration file for the Arduino code style is here: https://github.com/arduino/Arduino/blob/7754b7fc8a127b46ee099fbea8e8a71b70d3486d/build/shared/clang-format-arduino |
@per1234 Good to hear (great minds seem to think alike). I also looked into using CPPCheck because Cmake has some preliminary support for environments that already have that installed. However, CPPCheck seems to focus on preventing undefined behavior and not much about code style. Still it wouldn't be bad to use both in a CI workflow. |
Yep, clang-format is nice, it's just a bit of work to get nice results if you're trying to not cause too many changes in existing codebase. |
I just went through all the options and got a configuration that should match the style I've been seeing consistently (in RF24 libs) and from what I remember of NASA C style. 1 major problem that I've been fighting: the .clang-format file CANNOT be utf-16 (I think it only works with utf-8). Although I think this may be more specific to windows because I generated the file by exporting the webkit style rules (running clang-format 32-bit exe in powershell).
|
It was very challenging to try to write a ClangFormat configuration that matched the Artistic Style configuration used by the Arduino IDE's auto format. That astyle configuration was very relaxed. You want attached braces? Cool. You want them broken? No problem. (Or more likely) you want a random mixture of attached and broken? Sure thing. ClangFormat doesn't play like that. You can pick any possible single style you could imagine, but there is usually no way to just shut it off. In the end, we had to accept that it would not be possible to get a 100% match to the previous configuration. |
Also I just found out that some options aren't available in v12.01 (I got the prepackaged binary) while I was reading the v13 docs. There is no indication what options were added (or removed) between versioned docs, and I don't see a way to pull up v12 docs like I would for a readthdocs.org hosted docs |
It's a component of the URL. You can hack it to any version you need: |
FYI, wrt @per1234 comment about enforced extremely granular control, I'm forcing a_width = static_cast<uint8_t>(a_width - 2);
if (a_width) {
write_register(SETUP_AW, static_cast<uint8_t>(a_width % 4));
addr_width = static_cast<uint8_t>((a_width % 4) + 2);
} else {
write_register(SETUP_AW, static_cast<uint8_t>(0));
addr_width = static_cast<uint8_t>(2);
} becomes a_width = static_cast<uint8_t>(a_width - 2);
if (a_width) {
write_register(SETUP_AW, static_cast<uint8_t>(a_width % 4));
addr_width = static_cast<uint8_t>((a_width % 4) + 2);
}
else {
write_register(SETUP_AW, static_cast<uint8_t>(0));
addr_width = static_cast<uint8_t>(2);
} Any objections to this? |
@per1234 Have you decided how to integrate clang-format into CI workflows that previously used your I'm probably going to have to make a contribution to one of them (I like this one) to get an action that suits me. |
Here it is: |
@per1234 I recently helped @shenxianpeng develop a GH action that uses clang-tidy and clang-format to produce criticism in the form of annotations (and optional thread comments). I plan on integrating this action into all the RF24* repos (probably next year) as it can be configured to focus on
More info can be found in the "cpp-linter-action" repo's readme. Doesn't seem to be getting much widespread use yet, so it isn't as well vetted as some of the other clang-tools based actions (I was unimpressed with the previous offerings). |
Wow, taking a look at that soon, very interested in it. |
@Avamander I've incorporated the cpp-linter action in the "clang-format" branch. It doesn't use clang-tidy, but it is using a .clang-format file for all of the repo's source files. I've also used a separate .clang-format file (provided by @per1234) for use with only the arduino examples (which is also used in the PlatformIO CI job "check_formatting"). heads-up: It will be a nasty looking PR of file changes. I don't think clang-tidy is really necessary because our guidelines are so flexible and clang-tidy "rules" are not as flexible. However, clang-tidy output provides specific criticism (vs clang-format output's absent feedback) as to what linting rule was violated and how/where. So, I'm willing to devise a .clang-tidy file if numerous contributions prove that using clang-tidy would be more advantageous. ps - I've also added a .gitattributes file to force git to automatically convert windows style CRLF to linux style LF. 😉 |
I was a bit out of this discussion for a while. Recently I came back to use AStyle formatter in my projects but I suffer AStyle bug #541 astyle not finding project's .astylerc file.. Then I have found AStyle Project Status that the project will not be so maintained as it was before. The maintainer suggest - if someone wants to try something else - to use clang-format. @2bndy5, which formatter you configured to be used? Is it clang-format? |
the action I helped design uses
In practice though, I patched the action so that users can disable all clang-tidy checks and just run clang-format. I think the same could be done vice versa, but I haven't tried. Both tools work better when they have a compilation database to work with, but that can be generated from Arduino CLI tool (or preferably CMake). |
Locally, I've been using a vscode extension that runs clang-format ( |
Most source files contain a mix of tabs and spaces that renders in editors tab width different than 4 strangely. This is also the case on viewing the files online here on Github and applies also to the mesh and networking lib.
The text was updated successfully, but these errors were encountered: