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

Add .clang-format configuration file for source code formatting. #343

Merged
merged 2 commits into from
Jun 30, 2014

Conversation

jakepetroules
Copy link
Contributor

I went through every option in http://clang.llvm.org/docs/ClangFormatStyleOptions.html and picked the ones that seemed most consistent and sane.

I set ColumnLimit to 0 for now ("none") but do we want to set a line length limit? 80? 100? I know some people are pretty pedantic about this - we especially are in the Qt project :) - but personally I don't mind.

After we agree on a style and merge this we can auto format the codebase (minus third party code like bsdiff). It really is horrendous!

jakepetroules added a commit that referenced this pull request Jun 30, 2014
Add .clang-format configuration file for source code formatting.

Also run clang-format on the codebase.
@jakepetroules jakepetroules merged commit b60ae5e into master Jun 30, 2014
@jakepetroules jakepetroules deleted the clang-format branch June 30, 2014 21:38
@kornelski
Copy link
Member

I appreciate consistency, but massive reformatting is going to cause merge conflicts in open pull requests, such as migration to ARC, and will make it very hard for others' forks to merge back.

@jakepetroules
Copy link
Contributor Author

Yes, but you agree it still needs to be done at some point?

@kornelski
Copy link
Member

I think tabs to spaces can be done all at once now, because it's relatively easy to diff and merge with whitespace ignored.

@jakepetroules
Copy link
Contributor Author

OK, feel free to go ahead and do that (for source files - xcodeproj and plists should keep tabs).

After we merge pending pull requests we'll run clang-format. I did jump the gun a little here, sorry for that.

@MaddTheSane
Copy link
Contributor

If there was a way to have git update modified lines to match formatting rules…
On Jun 30, 2014, at 5:41 PM, Jake Petroules notifications@github.com wrote:

Yes, but you agree it still needs to be done at some point?


Reply to this email directly or view it on GitHub.

@kornelski
Copy link
Member

@MaddTheSane I actually do something like that with git add -p (or rather GitX's nifty UI)

Once you make changes you can apply formatting to entire file and then just stage the parts that are relevant.

@kornelski
Copy link
Member

Regarding the clang options:

  • AllowShortBlocksOnASingleLine: false 👍
  • AllowShortIfStatementsOnASingleLine: false 👍
  • AlwaysBreakTemplateDeclarations, Language: Cpp, Standard: Cpp03 — not applicable? There are other C++isms there and some settings which seem to target edge cases. Do we need all of them?
  • BreakBeforeBraces: oh dear. This is a holy war, and I'm in favor of K&R.
  • MaxEmptyLinesToKeep: 1 — not sure about this.

@jakepetroules
Copy link
Contributor Author

  • Language: Cpp - according to the documentation this is what you must use for C, C++, Objective-C and Objective-C++, so this stays
  • BreakBeforeBraces: not hard set on Allman, pick what you like - how about Linux (or Stroustrup) then? See http://clang.llvm.org/docs/ClangFormatStyleOptions.html
  • MaxEmptyLinesToKeep: 1 - why not sure? this simply prevents having multiple blank lines in a row

I'd just keep the C++ specific options for if we end up using any C++ code for some reason and/or if we get WinSparkle to merge with us. Clang is smart; it's not going to try to apply them to ObjC. :)

@kornelski
Copy link
Member

Regarding MaxEmptyLinesToKeep — I thought 2 lines were used to separate #include or @interface blocks from the rest of implementation, or 2+ lines used to separate groups of methods, but I see it's not consistently applied, so may be better to be normalized after all.

In latest code examples (I've checked HealthKit ObjC example and Swift book) Apple uses "Attach" style.

The Linux style is closer to what Sparkle does, and I'd be quite happy with that.

@jakepetroules
Copy link
Contributor Author

Linux is probably closest to what most Objective-C codebases do, too.

Never seen any codebase use two consecutive blank lines.

@kornelski
Copy link
Member

OK, great.

@kornelski
Copy link
Member

I've changed only the brace style.

I've tried to use BasedOnStyle: WebKit to inherit most options and just override the difference, but by default it formats pointers differently than Sparkle codebase, and clang-format from Alcatraz doesn't recognize PointerAlignment: Right option. Have you run into this problem?

@jakepetroules
Copy link
Contributor Author

Yeah, it didn't recognize the PointerAlignment options for me either.

@kornelski kornelski mentioned this pull request Jul 1, 2014
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

Successfully merging this pull request may close these issues.

3 participants