[Kyle's list] 8 - Source code formatting

_[as someone suggested this, I will open a thread for the main points Kyle brought up in this 007 megamail. First post is his original point. I could use some help with people adding the main points of what has been discussed since then.]
_

can we just decide on a consistent style and run a source code
formatter over the core once? now is a really good time for this,
because everyone is on the same page, so it isn’t going to cause any
conflicts. in the middle/end of the next development cycle would be a
bad time for this.

The Java K&R Style seems to be preferred by many/some. http://en.wikipedia.org/wiki/Indent-style#K.26R-style

kyle: the OF core is pretty similar to java style k&r, yes. i think if this
was any other open source project, everyone would be required to run
their code through the formatter with a specific preferences file
before committing. or there would be a single person dedicated to
merging that is responsible for reformatting. but i think either of
those things would slow down OF development so it’s probably going to
have to be something we just do once every release.

Hi folks,

I couldn’t find any place promoting some coding style for OF and I’ve noticed that the source seems pretty dirty in what comes to the coding style. That are files with tabs indentation, with spaces indentation, with trailing spaces, with several lines separating methods, etc.

If we don’t have such coding style I propose we start having one, for C++ I like Qt’s ( http://developer.qt.nokia.com/wiki/Qt-Coding-Style ) for example.

At least we should define the indentation and trailing spaces policy that I think are really important for the readability of the code:
* 4 spaces indentation
* No trailing spaces (there are git hooks and editors’ settings to warn us about this)

As the details on how to make it clean is sometimes a matter of taste, we can discuss and gives reasons why one style or another is better. The above is simply my suggestion and apart from the spaces over tabs choice (for the obvious reasons: it will always look the same whatever editor you use), if the indentation is 2 spaces or 4 spaces or if a { comes in a newline is more of a taste question like I said.

Related to this, we could gradually try to fix the files that don’t follow the coding style. Some people don’t like these coding style fixes because when they do git-blame, they can get all the lines of a file pointing to the person who did the coding style fix but I think that if this fix comes in a big commit (a whole file fix as minimum) upon doing a git-blame, one can just see if the commit refers to a coding style fix and then blame the file’s history previous to that commit.

Cheers,

I agree, there was a discussion on the dev list already, and the kinda/sorta agreement was near the K&R style. See the summary here http://forum.openframeworks.cc/t/[kyle’s-list]-8—source-code-formatting/6891/0

Homogenising code style would be a great achievement. are you aware of automatic tools for that, or is that gonna happen by hand? I think it’s probably gonna happen concurrently with the standardization of code comments (i.e. doxygen-compatible). Cf. http://forum.openframeworks.cc/t/[kyle’s-list]-5—documentation/6888/0

I would suggest continuing the discussion on these points in the respective threads, no need to fragment too much.

@bilderbuchi, i merged topics.

@jrocha, you’re my new best friend.

this is one of my biggest issues with OF – which is good, because it’s more of an aesthetic thing rather than a structural thing :slight_smile:

i tried to get the ball rolling on a consistent formatting style during ofdev-con earlier this year, and this is what we got http://piratepad.net/RejmoE8mhQ it’s not as comprehensive as the Qt spec, but it gives an idea for what the ‘feeling’ of OF is. notice that tabs is preferred. i agree with 80% of the Qt spec.

i think for this to happen, someone just needs to show that they can do it. everyone knows it needs to happen, the questions are just who will do it and when will it happen. more than ‘blame’, the worry is about work in branches being difficult to merge.

Yeah, I am used to projects like GNOME and MeeGo where coding style is taken seriously and virtually every source file looks the pretty much the same.

i tried to get the ball rolling on a consistent formatting style during ofdev-con earlier this year, and this is what we got http://piratepad.net/RejmoE8mhQ it’s not as comprehensive as the Qt spec, but it gives an idea for what the ‘feeling’ of OF is. notice that tabs is preferred. i agree with 80% of the Qt spec.

One thing, you mention K&R but you don’t separate keywords like if/while and their () with a space? e.g. it shows “if( smth==1 ){…” where K&R seems to do “if (smth == 1) {…”

I think that rather than defining a new one, we should perhaps choose one that is used for C+±based syntax such as Qt’s.

You said, that tabs are preferred. As I said, some things really are taste matter but can you tell me if there’s any advantage of using tabs over spaces?

i think for this to happen, someone just needs to show that they can do it. everyone knows it needs to happen, the questions are just who will do it and when will it happen. more than ‘blame’, the worry is about work in branches being difficult to merge.

I think many IDEs can define and apply a style to a whole file (I believe that Eclipse does so, last time I checked). I use Emacs which is not an IDE but there is probably a style formatting mode out there; at least it has some nice things like deleting all trailing spaces, untabify (self-explanatory) and trailing spaces highlighting.

As to the work related to branch management. We can lower the pain if we apply it to files that are not being touched, and when there’s a merge of a branch, those files could be reformatted readily afterwards.

Also, probably you might have seen my reply to how Git is organized and I really believe that if we followed my suggestions of a rebased branch per feature, our source code would look neat and it’d be easier to do the format changes.

Please note that I don’t have a history of participating in the OF community but I will to do so and I’m only suggesting these things because I think it can really improve OF as a FOSS project even if it requires a bit more discipline from code contributors.

Another important topic is how we would enforce/advocate the new coding style? how do we entice people to deviate from coding as they’re used to? A style guide everyone has to abide by, otherwise contributions get rejected? are there automatic checkers for that? jrocha, do you know how do the gnome guys do it?

In Eclipse, afaik, the code style you can indeed select only influences how generated code segments look like, not re-format existing code. could be wrong, though, and there’s a plugin I don’t know about.
From the help: "Use the Code Style preference panel to configure your global code style profiles for smart typing features, like auto-indentation and formatting. "
Eclipse offers K&R, BSD/Allman, GNU, Whitesmiths templates, but you can customize many aspects yourself.
Using a “known” code style (like K&R) instead of cooking one up would also increase the chances that the various IDEs used by people with oF (Eclipse, C::B, VS, XCode,…) support them already. Furthermore, chances are higher that people are already familiar with a known style, so it’s easier to follow it.

@Kyle: Why do you expect difficulties with merging branches?
@jrocha: I don’t want to derail the present discussion, but why do you think that using a rebased feature branch instead of a merged one (as it is now) would make it easier to format the changes? I fail to see a reason for that, the only thing being different is how the commit log looks like?

Whenever one contributes to a project. Even without having read the code guidelines, one should mimic the style present in the file that is to be changed.

What I have done to contributions that arrive to my projects with a different style (and that I assume that most people do) depends on how much it deviates from my coding style, e.g.:

  1. A patch comes and has just a few trailing spaces. I just correct these in the same commit.
  2. A patch comes and has unintuitive name (uses “w” instead of “window”, “dlg” instead of “dialog”). I just do a new commit on top of this one with the coding style fixes.
  3. A patch comes and messes up all the space/indentation/etc and variable naming. I reply to the sender asking him politely to change the code to fit the coding style.

An important thing to do always, even though we fix it ourselves, is to reply politely to the sender and tell them know what is/was wrong with the patch so hopefully next time things will be fine.

In Eclipse, afaik, the code style you can indeed select only influences how generated code segments look like, not re-format existing code. could be wrong, though, and there’s a plugin I don’t know about.
From the help: "Use the Code Style preference panel to configure your global code style profiles for smart typing features, like auto-indentation and formatting. "
Eclipse offers K&R, BSD/Allman, GNU, Whitesmiths templates, but you can customize many aspects yourself.
Using a “known” code style (like K&R) instead of cooking one up would also increase the chances that the various IDEs used by people with oF (Eclipse, C::B, VS, XCode,…) support them already. Furthermore, chances are higher that people are already familiar with a known style, so it’s easier to follow it.

I’m not sure but I think I once used something that was builtin in Eclipse that (upon saving) changed a Python file completely and applied the style I had configured.

@Kyle: Why do you expect difficulties with merging branches?
@jrocha: I don’t want to derail the present discussion, but why do you think that using a rebased feature branch instead of a merged one (as it is now) would make it easier to format the changes? I fail to see a reason for that, the only thing being different is how the commit log looks like?

Just take a look at what a merge does in Git as opposed to a merge *after* a rebase. Without presuming you don’t know your way on Git, for those who wish to know more about why rebasing is good, take a look at:
http://gitready.com/intermediate/2009/01/31/intro-to-rebase.html

My point was that *perhaps* it would cause less trouble to change whole files (coding style) if we used the rebase approach because whatever unformatted branches you might want to merge into a tree that has the format correction already in might be easier to first rebase them on top of those changes.
I was not only talking about the direct-merges VS rebase-and-merge, if we have the development of features per branches, before merging those, if we do a rebase we’d be applying the commits on top of what’s the latest changes instead of mixing it. After all, we’re interested in features/fixes getting applied on top of what is the current/working source and the info of whatever branch was merged doesn’t seem important, at least in the projects I mentioned which are really large and have many contributions.

Anyway, we can keep the discussion of the Git approach on another thread as this is about the coding style.

I’ve found that AStyle seems to be an automatic tool for stuff like that. According to their homepage, at least codeblocks supports this:
http://astyle.sourceforge.net/
ah, here the codeblocks plugin: http://wiki.codeblocks.org/index.php?title=Source-Code-Formatter-plugin

thank you for the article on rebase, especially the two others he links to had some interesting tidbits.
I agree, let’s keep the git discussion in another place.

edit:

Ha, turns out pressing Ctrl-Shift-F formats code you have selected according to the template…

Alright, so, how do we move on? Should there be a poll for choosing a style? Do people want to give opinion about whether some styling decision is good/bad (the tabs VS spaces for example).

If you want to check a good book about this, I suggest “Clean Code” by Robert C. Martin. It targets Java a lot but many things apply to C++.

I would propose simply using Qt’s style and refer to it when people want to know what coding style we use.
If you like Qt’s style but don’t want to refer to it directly because of whatever reasons, we could copy all or the most important parts.

Qt style looks reasonable. Some potential modifications:
I’d drop “Public classes start with a ‘Q’ (QRgb)”, I think that doesn’t make too much sense in our context?
I would prefer to break lines at 80, not 100 characters - 100 is awfully long.
“When reimplementing a virtual method, do not put the virtual keyword in the header file.” - I don’t understand that? do you have a choice about using keywords or not?? Or do they mean the filename?
Personally, I prefer keeping the opening braces of functions and classes also on the first line, but that’s no biggie.

The way forward:

  1. I think we wait a bit to give people not on the forum everyday opportunity to pitch in. If not much further input comes, I think we can agree on two or so candidates.
  2. Identify ways to automate compliance checks (e.g. Eclipse code formatter, codeblocks plugin, git commit options for trailing whitespace), see if there are premade configs suiting our style candidates.
  3. Based on 2), decide on one code style. Open a github wiki page, document code style information, oF-specific tweaks, ways to check one’s code, etc.
  4. Present to community. Take it from there (enforcement discussion, etc)

as to 1), I think the two most probable candidates are K&R Java Style, and Qt style. Barring any vetos, I think that’s the only two we have to seriously consider. Maybe even only Qt, since it’s documented in much more detail, K&R is indentation only, as far as i can see.

I think some kind of automation would be great, we don’t want people to refrain from contributing because coding style compliance is a pain in the a** to ensure. (also on integrator side). What do you think about that joaquim, is that a reasonable fear?

Of course. The Q refers only to Qt’s namespace, pretty much like we use “of” and “ofx” and I wasn’t proposing anything from Qt apart from the style.

To avoid such confusion maybe we should just copy most of the stuff apply our small modifications and put it in a wiki like you said.

I would prefer to break lines at 80, not 100 characters - 100 is awfully long.

“When reimplementing a virtual method, do not put the virtual keyword in the header file.” - I don’t understand that? do you have a choice about using keywords or not?? Or do they mean the filename?
Personally, I prefer keeping the opening braces of functions and classes also on the first line, but that’s no biggie.

I also prefer 80 chars instead of 100 although I agree that 80 is a historical measure and even split screen editors reach more than 80 chars long nowadays.

The virtual thing I think it refers to the cases where you don’t expect that method to be overridden by any inherited classes…

One thing I prefer that goes against what is stated in Qt’s style is to leave the operators in the previous lines when you need to split lines:
if (a +
b +
c) {…

But these are again tastes and we should just pick whatever most people feel more comfortable with. (I can take a look at the Clean Code book to check what the author advises in such cases)

The way forward:

  1. I think we wait a bit to give people not on the forum everyday opportunity to pitch in. If not much further input comes, I think we can agree on two or so candidates.
  2. Identify ways to automate compliance checks (e.g. Eclipse code formatter, codeblocks plugin, git commit options for trailing whitespace), see if there are premade configs suiting our style candidates.
  3. Based on 2), decide on one code style. Open a github wiki page, document code style information, oF-specific tweaks, ways to check one’s code, etc.
  4. Present to community. Take it from there (enforcement discussion, etc)

I agree. Let’s follow up in a couple of days.

tabs over spaces: 1 people have different preferences for rendering tabs, i like to render tabs as 2 spaces. 2 most IDEs use tabs as default 3 OF has historically used tabs.

k&r uses spaces after parens: yes, it does, i don’t think the github doc was really thinking about that. i’m not sure what the dominant approach in the current OF source is.

difficulties with merging branches: if you have a file in a branch, and then the core gets completely reformatted, you will have to manually merge any pre-format changes you’ve made because all the code you’ve been working on will have changed. one solution is to run the code formatter locally on your modified files before merging, this should minimize conflicts.

auto-formatting/plugins for formatting: i used to write a lot of java in eclipse, and i love the way eclipse handles this. astyle is nicely integrated with code::blocks and can be applied quickly from the plguins menu. nothing exists for xcode though, and the majority of OF development happens in xcode.

4-point list: awesome. completely agree, especially regarding copying the qt guidelines somewhere else we can edit them. and @jrocha operators on previous line: agreed.

i think the biggest trouble with coding style is for the integrators. arturo does most of the core development and integration, followed by theo, and then zach. zach doesn’t really integrate. i can see arturo enforcing coding style, but i think theo would integrate even less than he does if he needed to enforce a style. so it might make more sense for this to be something that’s encouraged rather than enforced. then we regularly do a ‘cleaning’ pass before a release. i’d like to hear what arturo thinks about this.

Ah I see now what you mean. I think having short-lived branches, or doing most of the reformatting in one swoop, would minimize this problem, too.

I just found this but can’t try, no Mac: http://eatmyrandom.blogspot.com/2011/03/xcode-astyle-part-2-for-xcode-4x.html

I have to disagree here. In addition to the fact that the Qt style doc makes a good point as to why operators should be on the next line, having them on the previous line also goes against mathematical convention as far as I’m aware of (being a physicist).

[quote=“kylemcdonald, post:14, topic:6891”]
i think the biggest trouble with coding style is for the integrators.[/quote]
Agreed. All the more reason to find some at least semi-automatic process (maybe git hooks are feasible for that: http://www.vtk.org/Wiki/Git/Hooks http://progit.org/book/ch7-3.html). I fear enforcement would scare away many contributors, so encouragement it is.

The important thing here is consistency. Tabs or spaces allow consistency, a mix of these is just a big mess.
Having tabs allows for people to define their length in their editor but my opinion is that code should look the same wherever you open it (with the obvious differences of syntax highlight and font). If you’re reading an online reference with examples of code and it uses tab-based indentation, there’s a good probability it will not look the same as in your editor. So using spaces just guarantees that the code will look the same in many editors/viewers and probably this is why projects like Python, GNU, GNOME, Qt…

Just in case someone wonders, of course this doesn’t mean people have to press the space bar a number of times for indentation but rather to configure their editor to use spaces for indentation and then just use the tab key as always.

difficulties with merging branches: if you have a file in a branch, and then the core gets completely reformatted, you will have to manually merge any pre-format changes you’ve made because all the code you’ve been working on will have changed. one solution is to run the code formatter locally on your modified files before merging, this should minimize conflicts.

auto-formatting/plugins for formatting: i used to write a lot of java in eclipse, and i love the way eclipse handles this. astyle is nicely integrated with code::blocks and can be applied quickly from the plguins menu. nothing exists for xcode though, and the majority of OF development happens in xcode.

4-point list: awesome. completely agree, especially regarding copying the qt guidelines somewhere else we can edit them. and @jrocha operators on previous line: agreed.

Let’s keep in mind that the reformatting is a bit painful but is supposed to happen only once for each file, not something that we’ll be doing over and over.

i think the biggest trouble with coding style is for the integrators. arturo does most of the core development and integration, followed by theo, and then zach. zach doesn’t really integrate. i can see arturo enforcing coding style, but i think theo would integrate even less than he does if he needed to enforce a style. so it might make more sense for this to be something that’s encouraged rather than enforced. then we regularly do a ‘cleaning’ pass before a release. i’d like to hear what arturo thinks about this.

Indeed. Let’s hear what the people that drive the project thing about this.
All I can say is that the code formatting task is not something for an integrator to do but rather the responsibility of any contributor, that is, you should always honor a project’s coding style. If people keep this in mind (as others do for many different projects), the coding style “enforcement” will be an occasional thing (someone forgot a trailing space, etc.) not a daily task.

[quote=“bilderbuchi, post:15, topic:6891”]

Take a look at my previous reply.
I’d like also to add that I contribute to different projects, with different coding styles and I have integrated also many patches in other projects and although first time/inexperienced contributors might commit more mistakes in what codes to the style, this hasn’t been a problem nor something that took all the time in the integration tasks.

Usually first time contributors don’t send you big scary patches so letting them know they need to change their 10-line patch is not really an issue and they will know how to do it next time.

I would be especially worried if a project didn’t consider a coding style because of scaring contributions, it seems like an overreaction and doesn’t justify having a littered code base. I could even argue that experienced people wanting to contribute for the first time to OF would find it harder if they saw that there’s no coding style and every other file look different.

source code should look the same everywhere: i agree, and this is a great argument for spaces. actually, one of the reasons i can’t stand alignment in code (aligned comments, aligned variables, etc) is because tabs can break it.

i think the bigger factors are the historical momentum within OF and IDE defaults.

my first reaction to your statement “code formatting is not a task for an integrator/it’s a one time thing” is that there’s no way that would be the case due to the huge variety of contributors to OF. so i decided to take a look at the list on github https://github.com/openframeworks/openFrameworks/contributors

maybe i’m wrong. i know a lot of these people and their style. i’m guessing 80-90% of contributed code would be correctly formatted. the rest could slowly be convinced.

in general it’s really interesting to watch this develop. there are a lot of open source communities, but i feel that OF is unique in that it has evolved from a bunch of non-CS people. it started as a smaller project with lots of irregularity and idiosyncracies, but as it gets bigger there is a certain degree of regularity that is required to keep communication clear and development easy. like you said, consistency is key.

+1 for coding style … I’ve been complaining about this for some time and have followed the rather loose style that’s already there. We just need to agree on something and DO it. It’s really not that hard. I’m willing to give up whatever idiosyncrasies I have and adopt something we can all live with.

My number one pet peeve, however is commenting. There is almost no real commenting in the core, beyond what little I’ve been able to add (ofLog https://github.com/openframeworks/openFrameworks/blob/master/libs/openFrameworks/utils/ofLog.h).

I really don’t see how writing in a consistent style and commenting your work is that much extra work, considering how much easier it makes maintenance and explaining what you did to others.

I agree about commenting, there should be more. Ideally, it would go hand-in-hand with the drive for doxygen-autogenerated documentation: http://forum.openframeworks.cc/t/[kyle’s-list]-5—documentation/6888/0