Skip to content
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

Closed
myTonino opened this issue Dec 6, 2017 · 72 comments
Closed

Mixed tabs and spaces #391

myTonino opened this issue Dec 6, 2017 · 72 comments

Comments

@myTonino
Copy link

myTonino commented Dec 6, 2017

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.

@Avamander
Copy link
Member

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.

@wmarkow
Copy link
Contributor

wmarkow commented Dec 23, 2017

@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:

  • K&R
  • BSD/Allman
  • GNU
  • Whitesmiths

@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.
NRF24 uses C++ so we would need some style guide related to C++ as well. Above I have put two link, the second one refers to NASA C++, I just found it in the Internet but I'm not quite sure if this is still valid. What are your thoughts?

@Avamander
Copy link
Member

Avamander commented Dec 23, 2017

@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).

@MAKOMO
Copy link

MAKOMO commented Dec 23, 2017

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.

wmarkow added a commit to wmarkow/RF24 that referenced this issue Dec 28, 2017
@wmarkow
Copy link
Contributor

wmarkow commented Dec 28, 2017

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.
I think there is something still missing in this formatter (like I have noticed that preprocessor commands sometimes are not formatted correctly). Most basic thing: I have put 3 spaces (NASA recommends strongly that) in the indentation. But we can use 4 spaces as well - I think.

@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.

@MAKOMO
Copy link

MAKOMO commented Dec 28, 2017

@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.

@Avamander
Copy link
Member

Avamander commented Dec 30, 2017

@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.

@wmarkow
Copy link
Contributor

wmarkow commented Jan 2, 2018

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:

void setup()
{
  // initialize digital pin LED_BUILTIN as an output.
  pinMode(LED_BUILTIN, OUTPUT);

  int  a = 5;
  if (a == 5)
  {
    a = 4;
  } else if (a == 3) {
    a = 2;
  }
}

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).

@Avamander
Copy link
Member

@wmarkow So you think we should format everything with the Arduino IDE?

@wmarkow
Copy link
Contributor

wmarkow commented Jan 2, 2018

No, I think we can stick to the NASA format as you suggested before.

Though the Arduino examples should be formatted with the Arduino IDE, should just be kept in mind when that bridge is to be crossed.

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.

@per1234
Copy link
Contributor

per1234 commented Feb 3, 2018

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:
https://github.com/arduino/Arduino/blob/master/build/shared/lib/formatter.conf

AStyle also allows you to configure more formatting options, as documented here:
http://astyle.sourceforge.net/astyle.html

I believe that adding the following options to the Arduino IDE's current AStyle configuration file will fully enforce the Arduino formatting style:

style=attach
break-closing-braces
attach-closing-while
attach-extern-c
add-braces
align-pointer=name
convert-tabs

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 break-closing-braces and attach-closing-while (which are not supported by that version) are required in addition to style=attach (which is supported).

Even though it's in the stock Auto Format configuration, I would suggest removing the keep-one-line-statements option because I think that just leads to code that's more difficult for beginners to read and more prone to bugs. I'm not aware of that coding style being used in any official Arduino code. And one statement per line certainly doesn't violate the Arduino formatting style.

@wmarkow
Copy link
Contributor

wmarkow commented Mar 5, 2018

@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:

if (a == 5)
{
  a = 4;
}

is correctly formatted, and also this is correctly formatted:

} else if (a == 3) {
  a = 2;
}

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:

  • automated process of formatting (like executing some script that will format all of the sources)
  • two different formatting styles: NASA formatter for RF24 library and Arduino formatter for the code of examples.

Going back to my exepriments with Eclipse formatter.
In my https://github.com/wmarkow/Arduino/commits/eclipse-formatter-poc branch I have just batch formatted the Arduino's examples with Eclipse formatter. The fromtter XML has been prepared by me by modifying the formatting rules in Eclipse and checking how it works on the Arduino's examples (I wanted to have as small changes as possible while formatting). You can take a look. Mostly it has reformatted the comments.
In my fork of RF24 library I have did the same in my https://github.com/wmarkow/RF24/commits/fix_for_%23391 branch. Examples are formatted with Arduino fromatter and other source code is formatted with NASA formatter. NASA fromatter rule for Eclipse have been prepared by me according to this NASA C++ guide.

@Avamander
Copy link
Member

Avamander commented Mar 5, 2018

@wmarkow The changes seem okay and really seem to remove quite a bit of uglyness, but I'm not sure about the char * array changes, new:

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?

@per1234
Copy link
Contributor

per1234 commented Mar 5, 2018

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 style=attach to enforce this.

I had thought the closing bracket and else statement were generally put on separate lines (break-closing-braces) but after looking through some official examples it appears that is incorrect. Both on the same line is more common in the examples included with the IDE, although not by an overwhelming majority. Without break-closing-braces my suggestion of attach-closing-while is no longer needed.

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.

@wmarkow
Copy link
Contributor

wmarkow commented Mar 7, 2018

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.
I have put some comments to my commit. Take a look wmarkow@af8c5d3 for reference.

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.
For NASA style we need to use style=allman. I have rechecked and style=attach works perfect for Arduino style - just like @per1234 has mentioned before.

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?
Old way of code:

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 new:

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,};

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.

@Avamander
Copy link
Member

@wmarkow Yeah, mixed up the words. I think the expanded version is more readable, what do you think?

@wmarkow
Copy link
Contributor

wmarkow commented Mar 7, 2018

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.

wmarkow added a commit to wmarkow/RF24 that referenced this issue Mar 9, 2018
wmarkow added a commit to wmarkow/RF24 that referenced this issue Mar 9, 2018
wmarkow added a commit to wmarkow/RF24 that referenced this issue Mar 9, 2018
wmarkow added a commit to wmarkow/RF24 that referenced this issue Mar 9, 2018
wmarkow added a commit to wmarkow/RF24 that referenced this issue Mar 9, 2018
wmarkow added a commit to wmarkow/RF24 that referenced this issue Mar 9, 2018
wmarkow added a commit to wmarkow/RF24 that referenced this issue Mar 9, 2018
wmarkow added a commit to wmarkow/RF24 that referenced this issue Mar 9, 2018
wmarkow added a commit to wmarkow/RF24 that referenced this issue Mar 9, 2018
@2bndy5
Copy link
Member

2bndy5 commented Sep 22, 2020

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.

@Avamander
Copy link
Member

I do prefer the readthedocs-sphinx theme over the doxy-gen output that http://tmrh20.github.io/RF24 uses.

Doxygen could be configured to use m.css that looks like this. But yes, this is a separate topic.

@wmarkow
Copy link
Contributor

wmarkow commented Oct 1, 2020

status on this?

I just did my tests with formatters like 2 years ago and that's it - from my side.

Pretty much done. Arduino examples are formatted using its IDE, code is formatted as agreed and new PRs are fixed before merging. It should be fairly consistent.

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.

well they prefer 8 space indentations...

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.

@2bndy5
Copy link
Member

2bndy5 commented Oct 21, 2020

If you want to take the astyle approach I mentioned earlier in this thread, you could use this workflow:
https://github.com/arduino/arduino-examples/blob/main/.github/workflows/code-formatting-check.yml
It's configured to only cover the example sketches, but only a tiny change would be needed to make it cover the library code files as well.

@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).

@per1234
Copy link
Contributor

per1234 commented Oct 21, 2020

After committing the changes from auto-formatter tool in the Arduino IDE, the workflow still marked all the same files as erroneous.

Here's the clue for why this happened:
https://github.com/2bndy5/RF24/runs/1286346121?check_suite_focus=true#step:5:25

Cannot open default option file /home/runner/work/RF24/RF24/examples_formatter.conf

You didn't add the formatter configuration file:
https://github.com/arduino/arduino-examples/blob/main/examples_formatter.conf
to the repo when you added the formatting check. This caused Artistic Style to use its default configuration, which doesn't match the Arduino formatting style.

You can see here the passing check in my fork:
https://github.com/per1234/RF24/runs/1289296630?check_suite_focus=true

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:
https://github.com/arduino/Arduino/blob/1.8.13/build/shared/lib/formatter.conf
I do like the "examples_formatter.conf" configuration though, so if you chose to have another try at this, you might use that configuration. You can use Artistic Style directly from the command line to format everything. I use this command:

astyle --suffix=none --options=examples_formatter.conf --recursive ./*.pde,*.ino,*.h,*.hpp,*.hh,*.hxx,*.h++,*.cpp,*.cc,*.cxx,*.c++,*.cp,*.c,*.ipp,*.ii,*.ixx,*.inl,*.tpp,*.txx,*.tpl

You can get astyle compiled for Windows here: https://sourceforge.net/projects/astyle/files/
and instructions for installing for other OSs here: http://astyle.sourceforge.net/install.html

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:
https://github.com/per1234/RF24/runs/1289283492?check_suite_focus=true#step:5:79

ERROR: Non-compliant code formatting in /home/runner/work/RF24/RF24/examples/scanner/output/scanner.cpp

This examples/scanner/output folder is an artifact of an ancient Arduino IDE version from when it used to save the build output to the sketch folder. maniacbug accidentally committed that to the repo 10 years ago, and there it has stayed ever since. Because it can cause confusion and maintenance burden (as we now have proof), this folder should be removed from the repository. However, I took the minimalist approach of just formatting the file.

With that, the check now passes:
https://github.com/per1234/RF24/runs/1289296630?check_suite_focus=true


Since the time I made that comment about the arduino/arduino-examples repo's formatting check workflow, I have made what I feel to be some improvements on the system, when I added this check to the SpenceKonde/megaTinyCore repository. You can see the workflow here:
https://github.com/SpenceKonde/megaTinyCore/blob/master/.github/workflows/check-code-formatting.yml
and the formatter configuration file used by the workflow here:
https://github.com/SpenceKonde/megaTinyCore/blob/master/megaavr/extras/ci/arduino-code-style.conf
Because SpenceKonde/megaTinyCore is a pretty big project with multiple distinct components, I broke the formatting checks into multiple jobs in that workflow, with the idea that this might make it easier to understand the results of a failing check. However, that's completely optional. I could have defined all the paths to check in a single step

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:
https://github.com/per1234/artistic-style-action

@2bndy5
Copy link
Member

2bndy5 commented Oct 21, 2020

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).

@2bndy5
Copy link
Member

2bndy5 commented Oct 28, 2020

@per1234 I'm loving the artistic-style-action. Ran into yet another yml snag: list of file formats requires a single quote ' where a double quote " caused an error saying expected to find an alphabetical or numerical character. Also I had to remove the option remove-comment-prefixes from the examples_formatter.conf file, but this is more of a continuity problem doxygen as the examples descriptions are actually located in the header file (which requires some extra copy-n-paste from example files' description to the header file that declares the example files).

I think I'm ready to call this issue resolved.

@per1234
Copy link
Contributor

per1234 commented Oct 28, 2020

I'm loving the artistic-style-action.

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.

Ran into yet another yml snag: list of file formats requires a single quote '

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).

I had to remove the option remove-comment-prefixes from the examples_formatter.conf file

That's not so bad. You're still pretty close to the standard Arduino formatting style. that remove-comment-prefixes is actually somewhat confusing because the Arduino IDE's editor automatically adds comment prefixes, but then its auto format removes them (arduino/Arduino#3941). So you sort of end up with a tug of war over the comment prefixes.

@2bndy5
Copy link
Member

2bndy5 commented Dec 7, 2020

assigned myself this issue (as a reminder) because it will likely be closed after merging #691

@2bndy5
Copy link
Member

2bndy5 commented Dec 14, 2020

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.

@2bndy5 2bndy5 closed this as completed Dec 14, 2020
@2bndy5
Copy link
Member

2bndy5 commented Aug 17, 2021

@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).

@per1234
Copy link
Contributor

per1234 commented Aug 17, 2021

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

@2bndy5
Copy link
Member

2bndy5 commented Aug 17, 2021

@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.

@Avamander
Copy link
Member

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.

@2bndy5
Copy link
Member

2bndy5 commented Aug 17, 2021

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).

ps - I think the clang-format config docs are a bit out-of-date

@per1234
Copy link
Contributor

per1234 commented Aug 17, 2021

it's just a bit of work to get nice results if you're trying to not cause too many changes in existing codebase.

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.

@2bndy5
Copy link
Member

2bndy5 commented Aug 17, 2021

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 ☹️

@per1234
Copy link
Contributor

per1234 commented Aug 17, 2021

It's a component of the URL. You can hack it to any version you need:
https://releases.llvm.org/12.0.1/tools/clang/docs/ClangFormatStyleOptions.html

@2bndy5
Copy link
Member

2bndy5 commented Aug 17, 2021

FYI, wrt @per1234 comment about enforced extremely granular control, I'm forcing else statements to have a break beforehand. It just easier to read (especially when skimming through) if the else statement has the same indent that the it's corresponding if statement has. So,

    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?

@2bndy5
Copy link
Member

2bndy5 commented Aug 17, 2021

@per1234 Have you decided how to integrate clang-format into CI workflows that previously used your per1234/artistic-style-action@main? I see adafruit uses a python script that calls clang-format as it walks through a directory. I see there are a few GH actions in the marketplace (one of them actually uses the same python script), but most recommend using another auto-commit changes action 👎🏼 .

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.

@per1234
Copy link
Contributor

per1234 commented Aug 17, 2021

Here it is:
https://github.com/per1234/.github/blob/development/workflow-templates/check-cpp.md
It looks like the action used is the one you mentioned that uses the Python script from adafruit/ci-arduino.

@2bndy5
Copy link
Member

2bndy5 commented Oct 18, 2021

@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

  • lines in the event's diff (only added or unchanged)
  • entire source files in the event's list of changed files (push and PR events supported)
  • all source files in the repo (which can further be configured with ignore & extensions options)

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).

@Avamander
Copy link
Member

Wow, taking a look at that soon, very interested in it.

@2bndy5
Copy link
Member

2bndy5 commented Oct 27, 2021

@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. 😉

@wmarkow
Copy link
Contributor

wmarkow commented Jan 6, 2022

Arduino decided to switch from Astyle to ClangFormat for Arduino IDE 2.x.

Yep, clang-format is nice,...

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?

@2bndy5
Copy link
Member

2bndy5 commented Jan 6, 2022

the action I helped design uses

  1. clang-tidy which is best used for syntax conformity and compilation errors
  2. clang-format which is best for linting syntax errors and possible points of undefined behavior.

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).

@2bndy5
Copy link
Member

2bndy5 commented Jan 6, 2022

Locally, I've been using a vscode extension that runs clang-format (alt + shift + F on windows). I just realized my previous answer was a bit promotional, but it is implemented in our clang-format branch (need to quell merge conflicts though).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants