-
Notifications
You must be signed in to change notification settings - Fork 278
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 file #152
Conversation
Note that this PR is a first step to address the issue #106 |
.clang-format
Outdated
SpacesInCStyleCastParentheses: false | ||
SpacesInParentheses: false | ||
SpacesInSquareBrackets: false | ||
Standard: Cpp11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This setting will reformat double closing >
braces in templates to >>
which will be interpreted as a shift operator in C++03.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, I did not notice that. I will change it. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just addressed this issue. I will wait for your approval to merge this ;)
Very good working through all the options clang format offers! (I am usually a billion times lazier and stick with only slightly modified defaults.) In principle I am very much in favor of this, as it would mean I can stop worrying about formatting and just press the key combo to format the source. But the diff is imho too big before a minor release. Maybe we could merge this after a bigger release? |
I would merge this now but without applying it to the whole code base. At my company we started to use clang-format 2 years ago (introducing a
We defined those guidelines after several iterations, and they are working pretty well in our case. I think it might be interesting to do something similar in Exiv2. When I write new code I like to have these utilities in place. |
That is actually a very good approach. It will keep the diffs reasonable and will just result in a gradual migration. |
src/bmpimage.cpp
Outdated
{ | ||
} // BmpImage::BmpImage | ||
} // BmpImage::BmpImage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why two blanks here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. We can control this with the field:
SpacesBeforeTrailingComments: 2
I will change it to 1.
But ... should not we just remove these comments ? I think they are useless. If there are functions so long that somebody can think it is a good idea to have such comments, I would rather think that we should refactor the code to have smaller functions that can be read easily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, good point to get rid of such useless comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, a good editor will tell you anyway and if you actually need such comments the function is too long. However, I'd keep these comments for namespaces and #ifdefs
Note that I am adding individual commits to the small changes I am doing while discussing about the final configuration. Once we agree in a final configuration, I will squash all the commits. |
Personally I do not like braces AfterControlStatement=true these days. It also introduces new lines in your example file. |
One other non-conservative option is the new line length limit 120. |
@tbeu I have changed the configuration so now it is based on the google style (as we discussed on slack). About the length limit, most of the existing files are respecting already this configuration. Examples:
Of course, there are others files that have lines exceeding this limit, and in some cases it makes sense to apply the formatting and in others not. |
I think this is good, the looks of the code will change, but it will be more consistent. What's your opinion on lowering the line limit while removing indentations for namespaces? This will penalize spaghetti code with lots of indentation more and force refactoring just for the sake of readability. The Linux kernel has such a policy (1 indentation = 8 spaces, line limit = 80 spaces). However the diffs will be bigger, as we currently indent namespaces. |
In my case I always work with indentations for the namespaces, 4 spaces for indentations and a limit of 100 or 120 chars for each line. I think that removing the indentations from Exiv2 would make a huge diff even for the simpler changes, but again, this PR is for discussing about all these things. My vote is for staying with the indentation for namespaces, but if there are more people preferring the other option, I will change it. |
I have no strong opinion on the namespace identation, but rather would prefer conservative formating, i.e., do not touch it for now. That's why I approved the PR as is. |
Guys, I'm merging this so we can give it a try while we touch existing code or create new one. If we see that we can refine a bit more the clang-format file, do not hesitate to create new PRs to update it ;) |
I think we should start the contributing.md file and document how clang format should be used. |
See http://dev.exiv2.org/projects/exiv2/wiki/Contributing_to_Exiv2 for what should go to Contributing.md. |
I created a new branch in which I started to write the |
Thanks for starting the work. Please make the PR sooner than later, as I suspect we'll have to discuss a lot of things. |
In this PR I propose the addition of a clang-format file for defining the style of the Exiv2 code.
I tried to be conservative and the options selected are matching (more or less) the style that was used in most parts of the project. Of course, we can discuss about some of these values and change them accordingly to our discussion :).
I also applied these changes to one complete file (that was small) and to few lines of other file. Modern IDEs (like QtCreator) have shortcuts to apply clang-format in a whole file or to the selected text.
You can find more information about clang-format in the following links: