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

Use clang-format on Travis #866

Closed
certik opened this issue Mar 12, 2016 · 7 comments
Closed

Use clang-format on Travis #866

certik opened this issue Mar 12, 2016 · 7 comments

Comments

@certik
Copy link
Contributor

certik commented Mar 12, 2016

I've heard from several different people that they simply enforce using clang-format in continuous integration. That way, there are no more discussions about what the right formatting should be --- the right formatting is the one specified in the .clang-format file, which tells clang-format how to format the source code. And the formatting is automatic.

Here is the documentation: http://clang.llvm.org/docs/ClangFormat.html, it integrates with Vim and Emacs, etc.

The pros are:

  • no more worrying/discussions about formatting (if we want to change formatting, we simply change the .clang-format file)
  • uniform/consistent, easy to read source code

The cons are:

  • it might become painful to contribute if you can't run clang-format easily. For example, as of now, I don't have it installed on any of my platforms where I develop. Sometimes I need to contribute a patch on some cluster, where clang-format is not available. So I would need to then go and reformat each patch before submitting a PR --- but each patch should conform to coding conventions anyway, so perhaps this is not an argument.
  • The same with submitting a patch online using github -- I've only done that for documentation, so again, probably not an issue.
  • it looks like different versions of clang-format provide different formatting, so all developers would have to use the exact same version, and then we all would have to upgrade when we upgrade on Travis. That sounds problematic.
  • also what about people who want to use Windows to develop, or other platforms. We need to make sure that clang-format installs everywhere.

We'll probably need to have a script that takes a PR, and reformats each patch. Then we can easily fix other people's PRs if they don't have access to clang-format. I.e. the author will fix the PR with regards to functionality, creates a nice set of patches, and then we just need to run our script from a command line that runs each commit through clang-format, resubmit as a new PR, and then merge.

So I think overall this might work.

This would fix #138, #764, #830, #831, #838.

Here is an example how another project implemented it and their documentation about it:

https://github.com/nest/nest-simulator/blob/2341e1d7209b28d9636fbed576ac3824a3e36087/build.sh#L136

https://nest.github.io/nest-simulator/coding_guidelines_c++

@htfy96
Copy link
Contributor

htfy96 commented Mar 12, 2016

I think we should do it step by step:
1.

  • Run cppcheck in .travis.yml and simply log error/warning messages due to false positives
    (I have done this several minutes before, and no warning was given)
  • Add .clang-format into this repository
    • Use clang-format to format existing files
    • Add clang-format into .travis.yml as well as style guide, so that build bot could detect inconsistent format

@srajangarg
Copy link
Contributor

How about astyle? Unix system installation and usage is as easy as

sudo apt-get install astyle
astyle -s4 -A3 --recursive symengine/*.cpp symengine/*.h

Other options are available here, we can customize it to our liking.
It is also available on MacOS and Windows.

@srajangarg
Copy link
Contributor

@certik
And this can be used to check on travis.
with git diff master instead of just git diff ? (I guess)

@certik
Copy link
Contributor Author

certik commented Mar 14, 2016

astyle doesn't seem to be an active project anymore. I would go with clang-format, which seems widely used and being updated.

@htfy96
Copy link
Contributor

htfy96 commented Mar 14, 2016

clang-format may not be a big problem for windows developers, because clang-format.exe could run individually.
http://rogeriodossantos.github.io/MainPage/Clang_-_Automate_Cpp_Google_Style_format_using_Clang-Format/
This author provides individual clang-format.exe and format.bat. Downloading and running it should just work.

@srajangarg
Copy link
Contributor

How reliable is this author's executable file? Also, what about newer versions. I don't think clang-format officially develops a windows executable.

@Sumith1896
Copy link
Contributor

Closed via #898

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

No branches or pull requests

4 participants