Uniformize code syntax

[gcode-importer]

Originally reported on Google Code with ID 128

see http://code.google.com/p/openjpeg/wiki/CodingStyle

Reported by detonin on 2012-02-20 16:22:14

[gcode-importer]

need to find a tool for indentation.

Reported by malaterre on 2014-02-25 16:01:43

• Labels added: Milestone-Release2.1
• Labels removed: Milestone-Release2.0

[gcode-importer]

See r2554 for suggestion

Reported by malaterre on 2014-03-03 16:13:44

[gcode-importer]

Is it possible / a good idea to setup a post-commit hook (pre-commit hook are not allowed
in GCode) to reformat the code if needed ? 

Reported by detonin on 2014-03-04 21:37:25

[gcode-importer]

This issue was updated by revision r2723.

Reported by malaterre on 2014-03-14 09:08:14

[gcode-importer]

Reported by malaterre on 2014-03-14 14:05:42

• Status changed: Started

[gcode-importer]

Reported by malaterre on 2014-03-14 14:05:57

[gcode-importer]

Could people comment on current coding style. See here: http://code.google.com/p/openjpeg/wiki/CodingStyle#Tabs

Reported by malaterre on 2014-03-14 14:06:42

[gcode-importer]

On my end,

I like tabs to be tabs. This allows anyone to see the code as they see fit with editor
options for tab-size.
Tabs shall only be used at beginning of lines.
Multiline declarations shall use a mix of tabs and spaces. Tabs shall be used to enforce
indentation as the first line. Spaces shall be used to extend indentation in order
to align following lines. Small example using \t for tabs.

\tchar foo[3x3] = {
\t                           0, 1, 2,
\t                           3, 4, 5,
\t                           6, 7, 8,
\t                         };

These rules guarantee that everyone can see indentations the way they want & keep proper
alignments.

Reported by mayeut on 2014-09-30 20:09:55

[stweil]

Indenting with tab characters (instead of spaces) is only an acceptable choice with a fixed tab width of 8 (that's the style used for Linux kernel code).

I personally prefer indentation with spaces, either in steps of 4 (preferred, used for many free software projects) or 2.

[boxerab]

This tool might be useful here: http://astyle.sourceforge.net/

an automatic formatter. You can specify the style you would like, and it will run through all source code files and re-format.

[stweil]

Yes. I can confirm that astyle works pretty good.

[detonin]

I gave astyle a try and pushed the result on the "codingstyle" branch, together with the config file I used (see tools/cstyle/opj_astyle.cfg in that branch). Plan is to merge it into master once we agree on the config file. Btw, is it better to do this just before the release or just after ?

Comments welcome.

There was also uncrustify proposed by @malaterre a while ago.
My opinion is that astyle works fine and seems simpler and lighter than uncrustify. Being simpler is an advantage if we want any contributor to process its PR through astyle before submitting.

@mayeut Do you think it would be possible to add a test in Travis to run astyle with the opj config file (might be in dry-run mode (--dry-run option)), and return a failure if there is any file being marked as changed ? The idea would be to merge PR only if it meets opj coding style.

[mayeut]

My comments, all this is only a personal preference of course.

• I'm ok with k&r (c.f. minor comment below) or allman style. I usually mix them which is bad... (k&r for short blocks, allman for longer blocks . I find it more readable mixing the 2 for some reason.)
• If spaces are to be used, I'd rather see it in steps of 2. I find this easier to read, especially with many indentations levels where lines are not fully readable (either cropped by limited screen width or wrapped depending on preferences on this matter)
• with astyle, I'd probably add --break-closing-brackets if k&r style is used (the following is from style documentation:

void Foo(bool isFoo)
{
  if (isFoo) {
    bar();
  } else {
    anotherBar();
  }
}
becomes (a broken 'else'):

void Foo(bool isFoo) 
{
  if (isFoo) {
    bar();
  }
  else {
    anotherBar();
  }
}

[malaterre]

The new kid in town is clang format. It is also nicely packaged in Debian.

[stweil]

@detonin, I suggest to start with existing pull requests which fix some smaller formatting issues (#678), then pull all other requests which are suitable for the next release, then make the release, and reformat the code at the beginning of the next release cycle.

Existing pull request will usually need more or less re-basing when the whole code gets reformatted, therefore it is good to apply them before that action.

I had a look on your astyle configuration and think that it is a good starting point. --convert-tabs is an important option needed to remove all tabs (not only at the beginning of lines). What about limiting the length of code lines (for example --max-code-length=80)?

I tried this set of options:

--convert-tabs
--lineend=linux
--indent=spaces=4
--style=kr
--exclude=thirdparty
--exclude=wrapping
--add-brackets
--max-code-length=80
--break-after-logical
--pad-header
--pad-oper
--unpad-paren
--recursive
--suffix=none

In most cases it seems to give good results, but astyle is not perfect, so some options (for example --pad-oper) sometimes fail to improve the code. That's why I'd not add a Travis rule to check code formatting (or only a simply one which checks for forbidden tabs, wrong line endings, lines which are too long).

[hjmjohnson]

personally, I am very excited to see this discussion. Providing consistent code is key to long term maintainability of a code base.

[hjmjohnson]

You might be interested in the discussion here: vxl/vxl#164

[julienmalik]

See also discussion here : vxl/vxl#145

[detonin]

@rouault let's go for the QGIS/astyle way then :)

[rouault]

See #919