Automatic C++ style for the Math library

This applies to anyone developing on the Stan Math library.

The basic C++ style has been enforced for a while with our continuous integration using cpplint.

With this pull request from @seantalts, we’ll be able to automate a lot of the formatting using ClangFormat. Installation instructions are below.

For our style guide, see the wiki page on Coding Style and Idioms.

Instructions for setting up clang format and git (from the pull request):


Setup

First, install clang-format

Editor setup

Emacs - there’s a plugin/script called google-c-style. You can find it here or just install from MELPA.
Spacemacs - you can add google-c-style to dotspacemacs-additional-packages.
Vim- check out https://github.com/google/vim-codefmt

Git hook

There is a pre-commit hook in hooks/pre-commit, and if you run bash hooks/install_hooks.sh it will be installed for you. Going forward, it will run clang-format -i (which implicitly uses the .clang-format file in the repo) on any changed files to format them. If you need to disable this for some reason (though your tests will fail if you haven’t formatted things correctly) you can use git commit --no-verify.


Thanks @seantalts!

4 Likes

Should we port this to Stan and CmdStan?

I’m going to edit the wiki to add this stuff to it. I also need to add the tests to Jenkins to check if it’s formatted correctly.

1 Like

Thanks! Does this mean we can remove cpplint?

I think yes. Perhaps give the main devs on each repo a chance to evaluate?

I’m not 100% sure that changing the whitespace in the Stan repo will be ok, but it’s a quick(ish) check. I thought some of the funky whitespace was due to compiler bugs / behavior with most vexing parse or maybe something else. I’d also like to get @Bob_Carpenter’s take on the automatically formatted code after it’s applied to the compiler. I wouldn’t be surprised if there are minor tweaks to make to the formatting rules more palatable.

A cpplint error just came up on Yi’s pull request because of a missing include in a test file. Will the reformatting test for missing includes? Do we care? We have header tests that will test independent compilability.

No, clang-format doesn’t test for missing includes. We should probably also keep cpplint around for that and whatever else it does in addition to formatting stuff (hard to actually find a list anywhere).

uhm… I just tried to set this up and I get:

[10:20:36][sebi@sebastians-macbook-pro-1:~/work/math/stan/math/rev/mat/functor]$ clang-format -i map_rect_reduce.hpp
YAML:35:27: error: unknown key 'SplitEmptyFunctionBody'
  SplitEmptyFunctionBody: true
                          ^~~~
Error reading /Users/sebi/work/math/.clang-format: Invalid argument

I grabbed clang-format 5.0 from macports. My version is:

clang-format version 5.0.0 (tags/RELEASE_500/final)

For the moment I will disable that rule… but anyone has an idea on this? What clang-format version should I use? I figured that the latest is best.

Thanks. (this auto format is great should it work).

Weird, mine, also on mac but installed through homebrew, claims to also be version 5.0: clang-format version 5.0.0 (tags/google/stable/2017-06-22). I could remove the problem key from the file - just one formatting rule we’d have to follow by hand.

I removed the problem key on develop. I’ve been updating the wiki with instructions and whatnot and keeping that up-to-date as the authoritative store of the information about auto-formatting and style. The latest addition is an instruction to install a specific version of clang-format for Mac so we don’t have any inconsistencies.

Jenkins now tests with this specific version, though I need to get the correct version installed on the master node which homebrew doesn’t support anymore…

Just did my first bit of clang-formatting. I was skeptical, but this is awesome! I can just type out comments/code however I want and then the formatter fixes them! No worrying about rearranging code to fit on 80 lines anymore.

1 Like

How did you get the right version of clang-format installed on Ubuntu?

I went by the wiki here. My .bash_history shows:

sudo add-apt-repository "deb http://apt.llvm.org/xenial/ llvm-toolchain-xenial-5.0 main"
sudo apt-get install clang-format-5.0
sudo update-alternatives --install /usr/bin/clang-format clang-format /usr/bin/clang-format-5.0 100

And then check with:

clang-format --version

edit: Ubuntu 16.04, for what it’s worth. You gotta get the right repo link for whatever version you’re on.

Which version do you have? I have 5.0.1 and it’s not good enough for our CI so I can’t finish a PR right now.

bbales2@frog:~$ clang-format --version
clang-format version 5.0.0-3~16.04.1 (tags/RELEASE_500/final)

What’s the error?

That’s all I get:

[ClangFormat] test/unit/math/fwd/scal/fun/gamma_p_test.cpp is not formatted correctly according to clang-format
[ClangFormat] test/unit/math/prim/scal/fun/grad_reg_lower_inc_gamma_test.cpp is not formatted correctly according to clang-format

My local version is:

krzysztof@hagrid:~$ clang-format --version
clang-format version 5.0.1-svn319952-1~exp1 (branches/release_50)

It’s what the LLVM ppa’s get you on:

krzysztof@hagrid:~$ cat /etc/lsb-release 
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=17.04
DISTRIB_CODENAME=zesty
DISTRIB_DESCRIPTION="Ubuntu 17.04"

I was going to look into installing the specific tag (google/stable) from source but it’s a beastly download and I haven’t looked up how to get the right tag using csv yet, I think I know how from the git mirror.

The only clang-format command I’ve been executing is:

clang-format -i test/unit/math/fwd/scal/fun/gamma_p_test.cpp

That should fix the formatting of the file in place. Are you getting that error on commit or some other command? Maybe we should summon @seantalts.

No, I’ve done that command and my version of clang-format is fine with it, but it’s a different version than on Jenkins and they don’t agree!

Oooooooooooooowwwww. So I think my answer to your original question then is I got lucky :/.

Why are things hard? :(

All the usual suspects! Versioning. Floating point. Everyone working on different OS-es. C++'s spec that leaves way too much to the compiler writer’s discretion. Moving targets as C++ and the library dependencies change.

Every dependency just adds that much work. Everyone says each one’s easy, but they add up and eventually we hit the limit where all of our time is spent on maintenance. This is why I’ve been so against adding dependencies beyond Boost and Eigen. Even so, it’s been a nightmare keeping up.

1 Like

I’m convinced! I’m convinced!