I couldn’t stick around for the discussion at the meeting today, but @seantalts, mind summarizing here if you got around to talking about it?
Sure thing.
I got buy in to change to vanilla Google style guide, with the condition that I set up clang-format to convert the existing code as much as possible over, and ideally also set it up to run as a git hook or something similar.
The benefits of this approach are:
- less reading / information to keep in mind for contributors
- single easy reference and 3rd party authority during pull requests
- a widely-used industry standard that more people would likely be used to (vs our custom ideas about style)
- an automated formatting tool (which I’ve had in the past with Go’s gofmt and been very impressed. I think this is huge).
The idea is to pick some relatively quiet point in time to convert the entire repo over, because many people think we couldn’t switch coding standards without changing all of the existing code to mostly follow those standards (as much as we can in an automated way, anyway).
Bob brought up one worry, which is that right now Stan doesn’t do any name translation between the Stan language and math repo functions, whose names would likely be changed by clang-format.
I don’t think I’ll get to this soon, unless I start using it to procrastinate on other more important stuff I have going on.
2 Likes
Awesome! I’m all for having more enforceable, scriptable rules where the reviewer doesn’t need to think about style as much.
That’s great. Most of it we’ll be able to follow, but maybe take a closer look at our exceptions to Google’s style guide?
I think the exceptions thing is gonna be tough, and the boiler-plate is just gonna be annoying, but we could change it.
I’m mostly interested in using the Google style guide as the authoritative source for style in code reviews so we can spend less time discussing how things should be formatted, and also in using their automated tooling for formatting. You raise a good point that the guide also goes into more functional issues, like whether to use exceptions or complicated templates. I was imagining just using the guide for pure style things like formatting and naming. I think we should keep our functional diversions from the style guide as-is and leave our use of exceptions and template metaprogramming alone.
2 Likes
That sounds great!
A lot of those exceptions were options we passed to cpplint.
We should be able to update the options in one of our makefiles.
The exceptions to Google style that we outlined on the wiki page was disabled here in make/cpplint: https://github.com/stan-dev/math/blob/develop/make/cpplint
Removing those flags should help a bit.
Thanks, @seantalts!
Do I understand this right in that you suggest to automatically format the source by some code beautifier which automagically will put stuff in the right format? That sounds great to me. I really do not like this chasing those extra spaces, being blamed for missing returns, etc…
How would developers then code? We write the code and then how does it get automagically correctly formatted?
This sounds all in all like a very good suggestion.
Yeah, I haven’t tested it but there is a tool called clang-format that should just automatically format things appropriately, assuming the code is syntactically correct. This would be a nice standardizing feature and should lessen the burden on both developers and reviewers around some of these simpler style guide issues that can be automated.
1 Like
If we follow their naming conventions, pretty much every function is going to have to be renamed. This will mean a wholesale breakage of backward compatibility. And that means only on a major version release.
I don’t know what it does when we then use libraries like Eigen or Boost or the standard template library that don’t conform to Google style.
Tool is here:
https://clang.llvm.org/docs/ClangFormat.html
Style options here:
https://clang.llvm.org/docs/ClangFormatStyleOptions.html
They have emacs and vim modes.
Google format isn’t the only one out there.