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

Introducing Clang format #89

Merged
merged 3 commits into from
May 19, 2020
Merged

Introducing Clang format #89

merged 3 commits into from
May 19, 2020

Conversation

JensWehner
Copy link
Contributor

This is a bit of a speculative PR:

So for most software packages have specific formatting guidelines to ensure consistency. To make the code adhere to these guidelines, formatter programs are used. Clang-format is such a program.

Running make format now will reformat the whole code to adhere to the guidelines. This also allows Travis to later on check that submitted pull requests adhere to the same guidelines.

The guidelines themselves are given in the .clang-format file, which can be modified according to your taste. The 5 major conventions can be seen here https://gist.github.com/andrewseidl/8066c18e97c40086c183. At the moment I just added the google formatting conventions.

@yixuan
Copy link
Owner

yixuan commented May 6, 2020

Thank you. I'm happy to make this move.

@yixuan
Copy link
Owner

yixuan commented May 6, 2020

I guess we also need to exclude some files such as test/catch.hpp.

@JensWehner JensWehner marked this pull request as ready for review May 6, 2020 12:14
@JensWehner
Copy link
Contributor Author

catch.hpp is excluded at the moment because it only takes .h files.

Are you happy with that formatting guideline?
Because in that case, it would be best to once format the whole repository. I would do that in a another PR. It would touch all files once.

@yixuan
Copy link
Owner

yixuan commented May 6, 2020

I'll tune those options a little bit and give you my choices later.

@yixuan
Copy link
Owner

yixuan commented May 11, 2020

Hi @JensWehner, sorry for the long wait. It took me a while to learn how CMake works and to configure clang-format options. I put my version in the clang-format branch, and you can see if it looks acceptable to you.

I know these are not the most popular styles, but I chose the options such that the formatted code does not differ too much from the existing one. I also needed to guard some code blocks using // clang-format off, since the formatted version did not look nice.

@JensWehner
Copy link
Contributor Author

HI @yixuan . No worries, in the end it is up to you. As long as there is a guideline I do not care. From my side you can merge it into master.

The only thing I would reconsider, is using clang-format off for many areas, because then it defeats the purpose. Of course for specific and unique situations it can be a good idea. In the end you call the shots. :)

@yixuan yixuan merged commit 1c24dfd into yixuan:master May 19, 2020
@yixuan
Copy link
Owner

yixuan commented May 19, 2020

Hi Jens, thanks for your suggestions. I've further tweaked the configuration and now I get rid of most of the manual guards, except for a few cases that have heavy comments for class members. I have applied clang-format to the code in the new version.

Also I created a discussion thread (#92) for the future development of Spectra. Feel free to leave your thoughts there.

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.

2 participants