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

Using Clang-Format for Automatic Source Code Formatting #71

Closed
pratikpc opened this issue Feb 5, 2019 · 7 comments
Closed

Using Clang-Format for Automatic Source Code Formatting #71

pratikpc opened this issue Feb 5, 2019 · 7 comments

Comments

@pratikpc
Copy link

pratikpc commented Feb 5, 2019

In many portions of the Library we find that an inconsistent C++ Code formatting style is followed.
For example, certain places have

  1. Different Tab Sizes
  2. Different Brace Styles(Allman vs 1TBS/K&R/Stroustrup)

We could use an Automatic Source Formatter to rectify such issues.
While I would recommend Clang-Format, AStyle also exists

We could create a configuration file and define the properties.
For a sample Clang-Format configuration, see this
We could decide on what the properties would be here.

To simplify our tasks and reduce the conflicts that come with the question, Clang-Format also provides a few default styles like:-

  1. LLVM
  2. Google
  3. Chromium
  4. Mozilla
  5. WebKit
@per1234
Copy link
Contributor

per1234 commented Feb 5, 2019

I think something like this would be very helpful. Sometimes I spend more time trying to figure out how to make my code follow the unclear conventions set by inconsistent code formatting than I do writing the actual code.

A CI check for code style compliance should be done on every PR and commit to enforce consistent formatting from the start, rather than having to periodically go back and clean up whatever mess has accumulated since the last run of the formatter. The person writing the code should be responsible for writing compliant code and the code shouldn't get merged until it passes the check. That should be quite easy to set up.

I prefer AStyle to Clang-format. The reason is that the Arduino IDE already uses AStyle for the Tools > Auto Format feature. There is an AStyle configuration file used for formatting of the example sketches which should be consistent with the Auto Format configuration, but with additional style requirements. That file provides the closest thing to a code style guide that Arduino has:
https://github.com/arduino/Arduino/blob/master/build/shared/examples_formatter.conf
Newer versions of AStyle than the Arduino IDE uses permit making the code format rules even more specific.

I would like to see a consistent code style used across all Arduino code that runs on microcontrollers, whether that is example sketches or library/core code. Ideally some of the same conventions would be extended further into the Arduino code base.

@pratikpc
Copy link
Author

pratikpc commented Feb 6, 2019

AStyle will work well as well.
Although we probably would require a more stricter version of AStyle than what passes off as formatting in Arduino IDE(1.8.x) which tries to be inclusive to multiple styles rather than one for obvious reasons.

Rather than making it difficult for the person, I'd say we pass an AStyle or a Clang Format file as part of this repo in the main folder which every person has to run before committing, no excuses and then have a web based CI handling the verification.

I am supportive of standardisation as well.
It makes reading and writing code easier for everyone. If suppose someone were to dislike the standard this community chose and have a preference for another style, they could run their very own file for formatting purposes and then run the AVRCore formatting before uploading

While I too would love to see a consistent coding style across all Arduino repos, I believe we have to at least start somewhere first. Also doesn't the Arduino IDE use Java? In which case they would for obvious reasons prefer another coding style which might not really suit C++.

As such, I believe here we should maybe discuss here about a One True Standard for this Repo?

Should I update the Title to mention AStyle rather than Clang Format??

@facchinm
Copy link
Member

facchinm commented Feb 7, 2019

I'd love if all the code was coherently formatted but this would break some (most) of the PRs. I know that this can be solved by merging (or closing) existing PRs but we are having an hard time doing this (mostly because the current "core" focus is on validating ArduinoAPI and porting the various cores to it).

Said that, a huge PR with all the code reformatted and a companion script that can be added as a git precommit hook would not hurt anyone 🙂

@per1234
Copy link
Contributor

per1234 commented Feb 7, 2019

Also doesn't the Arduino IDE use Java?

Yeah, I wasn't suggesting that a single code style would be used across all the repositories, though I do think it would be cool to have consistency where possible. However, that is expanding the scope to the point where probably no consensus could be reached. I guess we should restrict the scope to C and C++ code. My earlier comment was more thinking about eventually applying it to C/C++ code used by Arduino for other purposes. The only such project I know of is arduino-preprocessor. Even if we restrict this to microcontroller code, that will be great.

I think just establishing an official code style and making a good effort to follow it on all new commits would be a very good step towards the goal. The CI check I proposed would be difficult to implement until the existing code is all compliant.

I have an AStyle configuration for a strict enforcement of what I've been able to decipher as the "Arduino Style" which I put a reasonable amount of thought and research into a while back for a personal project. I want to look at it a little more closely before presenting it as my proposal. It's quite difficult to interpret a style from code that is not consistent. I think of the examples bundled with the Arduino IDE as the primary source, and these are reasonably consistent since they've been through the auto-format. The problem is that they don't contain any precedents for certain things. Once you start diving into the core and library code looking for a specific style, things get much more difficult because there is a lot of inconsistency. In that case, some arbitrary decisions might need to be made. If we can stipulate that the existing auto format settings are already set in stone, that will greatly decrease the things that are left for people to disagree on. Otherwise, we'll probably never get past the tedious spaces vs tabs and if spaces, how many? arguments. I only desire consistency. I leave my own formatting preferences behind when working on a collaborative project.

@facchinm do you think this is the correct place to have the discussion of the official code style? Since this will eventually have a larger scope than Arduino AVR Boards, it seems more appropriate for the arduino/Arduino issue tracker.

Surprisingly, I don't find an existing issue about this. The closest would be arduino/Arduino#6503. I remember the subject came up at least a couple times in the Developers Mailing list, but without any sort of resolution:

@per1234
Copy link
Contributor

per1234 commented Feb 24, 2019

Here's my proposed AStyle configuration. I have commented it to indicate the origin of each option (formatter.conf is used for the Arduino IDE's Tools > Auto Format, examples_formatter.conf is used by a script in the arduino/Arduino repository which does bulk formatting of example sketches) and you can get information on all the options in the AStyle documentation.

# formatter.conf, examples_formatter.conf
mode=c


# examples_formatter.conf
# http://astyle.sourceforge.net/astyle.html#_style=java
# Considering changing this to the synonym "style=attach", which seems more descriptive
style=java


# examples_formatter.conf
attach-namespaces

# examples_formatter.conf
attach-classes

# examples_formatter.conf
attach-inlines

# examples_formatter.conf
attach-extern-c


# formatter.conf, examples_formatter.conf
indent=spaces=2

# formatter.conf, examples_formatter.conf
indent-classes

# formatter.conf, examples_formatter.conf
indent-switches

# formatter.conf, examples_formatter.conf
indent-cases

# formatter.conf, examples_formatter.conf
indent-col1-comments

# examples_formatter.conf
indent-modifiers

# examples_formatter.conf
indent-namespaces

# examples_formatter.conf
indent-labels

# examples_formatter.conf
indent-preproc-define


# formatter.conf, examples_formatter.conf
pad-header

# formatter.conf, examples_formatter.conf
pad-oper

# examples_formatter.conf
unpad-paren


# formatter.conf, examples_formatter.conf
remove-comment-prefix

# formatter.conf, examples_formatter.conf
# http://astyle.sourceforge.net/astyle.html#_keep-one-line-statements
# "Don't break complex statements and multiple statements residing on a single line."
# I don't like one line complex statements, but I guess since it's in formatter.conf it must stay.
keep-one-line-statements



# Options from examples_formatter.conf that I think should be removed:

# http://astyle.sourceforge.net/astyle.html#_indent-preproc-block
# "Indent preprocessor blocks at brace level zero and immediately within a namespace. There are restrictions on what will be indented. Blocks within methods, classes, arrays, etc., will not be indented. Blocks containing braces or multi-line define statements will not be indented. Without this option the preprocessor block is not indented."
# This does indent for #ifdef, but not for #ifndef, so it's quite inconsistent
# Indentation of preprocessor directives as done by this option is not very common in Arduino AVR Boards core, and where it is used, it's typically done inconsistently throughout the file
indent-preproc-block

# http://astyle.sourceforge.net/astyle.html#_indent-preproc-cond
# "Indent preprocessor conditional statements to the same level as the source code."
# Indentation of preprocessor directives as done by this option is very rare in Arduino AVR Boards core
indent-preproc-cond



# Options I have not implemented from formatter.conf or examples_formatter.conf:

# examples_formatter.conf
# Not a valid option in the latest version of AStyle. I think the correct option name is "add-braces", which I do use in my configuration
# add-brackets

# formatter.conf, examples_formatter.conf
# Not a valid option in the latest version of AStyle.
# indent-preprocessor



# Options I have added:

# http://astyle.sourceforge.net/astyle.html#_add-braces
# "I believe this is the correct option name to use instead the "add-brackets" option used in examples_formatter.conf. "add-brackets" is not a valid option in the latest version of AStyle"
add-braces

# http://astyle.sourceforge.net/astyle.html#_convert-tabs
# "Converts tabs into spaces in the non-indentation part of the line."
# AStyle is already configured to use spaces for indentation by indent=spaces=2. The Arduino IDE uses spaces instead of tabs by default.
convert-tabs

# http://astyle.sourceforge.net/astyle.html#_attach-return-type
# "Attach the return type to the function name in function definitions."
attach-return-type

# http://astyle.sourceforge.net/astyle.html#_attach-return-type
# "Attach the return type to the function name in function declarations."
attach-return-type-decl 

# http://astyle.sourceforge.net/astyle.html#_align-pointer
# "Attach a pointer or reference operator (*, &, or ^) to either the variable type (left) or variable name (right), or place it between the type and name (middle)."
# In Arduino AVR Boards core, name alignment of pointers is somewhat more common, though all possible styles are used
# I don't care which style is chosen (type, middle, name), but I do think one should be chosen and used.
align-pointer=name

Although they are in my configuration, I recommend that the options indent-preproc-block and indent-preproc-cond be removed. These options are only present in examples_formatter.conf. None of the built-in examples use preprocessor conditionals, so they aren't affected by these options. In the example sketches of the bundled libraries, only the Adafruit Circuit Playground examples use the style enforced by these options. In the Arduino AVR Boards core, indentation of preprocessor conditionals is not common, and done inconsistently in the files that do have it. The style enforced by indent-preproc-cond is very rare in those files.

There are two options that I have omitted from my configuration. The reason is that they are not valid options in the current version of AStyle.

I have added five options which are not present in formatter.conf or examples_formatter.conf. These are at the bottom of the file.

I'd also like to see a style guide for things that can not be enforced via AStyle. Here's just a quick list of things off the top of my head. Popular style guides can serve as a reference for further ideas:

  • const correctness
  • comment style
    • Number of tabs before an inline comment.
    • Single space between // and the comment text.
    • Don't capitalize first letter of comment text.
  • Only do alignment of comments/code when it provides a clear benefit. When it's done arbitrarily, it just makes the code more work to edit and can require unnecessary changes to lines unaffected by the code change just to adjust their alignment.
  • Use of blank lines.
  • End all files with a single newline

FYI, this is me wearing my volunteer hat

@Floessie
Copy link

  • const correctness

:+100: (The other points are also important, but this one stands out, as it affects the code and isn't just about readability.)

One thing that bothered me last time I used AStyle was its handling of C++11 initializers. Don't know if it's ready for that yet.

Every code formatter has its "challenges". One which can be adjusted to circumvent most of them is uncrustify. But even then there might be places where well intended formatting gets destroyed, so manual inspection is always necessary.

Just my 2¢,
Flössie

@aentinger
Copy link
Contributor

The topic of automatic code formatting comes up on a regular basis and is usually intensely debated. The major concern is breaking git blame when performing automatic formatting of an already established codebase. The most current automatic code formatting proposal is being discussed here.

As of right now I can only suggest to use asytle in combination with examples_formatter.conf when creating your own libraries since it is easier to enforce and maintain a certain coding style when you are starting on a green field. A example on verification of coding style via astyle and Travis CI can be found here (Thanks to @per1234 for coming up with it ;) ).

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

No branches or pull requests

5 participants