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

Fix C coding style #270

Closed
ignatk opened this issue Dec 25, 2017 · 9 comments · Fixed by #397
Closed

Fix C coding style #270

ignatk opened this issue Dec 25, 2017 · 9 comments · Fixed by #397
Labels
core Themis Core written in C, its packages enhancement

Comments

@ignatk
Copy link
Contributor

ignatk commented Dec 25, 2017

Many of Themis .c files have different code style (or no code style at all?). This creates confusion for potential contributors. Let's fix one coding style for all Themis/soter (not imported) .c files. My proposal is to use clang-format tool:

  • discuss and select initial coding style
  • fix it by writing a .clang-format and commit it to the repository
  • format Themis .c files with it
  • maybe, create a Makefile target, which developers can call to format all Themis/soter sources (in case they add additional files in the future)
  • finally, integrate format validation in CI (basically, fail the CI if code is not properly formatted) to enforce proper coding style for contributions
@vixentael vixentael added enhancement core Themis Core written in C, its packages labels Dec 25, 2017
@vixentael
Copy link
Contributor

I think it's a great idea. I think we should try to format using LLVM or Google pre-defined styles first, just to understand how far we are from their code style, then to apply @secumod step-by-step :)

@ignatk
Copy link
Contributor Author

ignatk commented Dec 26, 2017

In clang-format you can inherit the style and just override parameters you need - that's the best approach IMO. For example, you can define in your .clang-format you're using llvm style as a baseline, but override indent width (they use 2, but I like 4 for example).

Here's a nice interactive tool to understand what each setting means
https://zed0.co.uk/clang-format-configurator/

@ignatk
Copy link
Contributor Author

ignatk commented Dec 26, 2017

Generally I'm fine with llvm style, just generally prefer IndentWidth to be 4. However, I'll not fight for it, because sometimes 2 is handy: when you have multilevel nested code, you generally have more room for nested levels.

@gene-eu-zz
Copy link

Awesome idea. It's important to investigate the volume of effort to achieve that to prioritize this along other goals we've got for next release. Asking @vixentael or @Lagovas to weigh in with well-investigated opinion on how long will it take to do this.

@Lagovas
Copy link
Collaborator

Lagovas commented Dec 26, 2017

never worked with this utility and doesn't know how they will work with current code formatting. it can be like one-time run and forget, or run and fix in many places bad readable formatting. if we have time then it will be good. if not, then it's good to start to use it incrementally on changed files in commit

@ignatk
Copy link
Contributor Author

ignatk commented Dec 26, 2017

Actually the readability is OK. I just encountered that I needed to refactor includes in main themis.h header: the problem was if includes are grouped together without extra newlines, clang-format sorts them. In our case, it have put themis_errors.h last, so the compiler failed when parsing other headers, because it could not find themis_status_t definition.

Fixed it by moving themis_error.h higher and adding a newline.

For complete example, see this branch: https://github.com/secumod/themis/tree/clang-format/src/themis

I reformatted files in src/themis with clang-format default llvm style.
It did not break the build it seems: https://circleci.com/gh/secumod/themis/50

@Lagovas
Copy link
Collaborator

Lagovas commented Dec 26, 2017

vote for pointer on type instead variable name :) (int* a instead int *a)
exception is case with many variables like int *a, *b, *c;
but I think it will be too complicated for automated tool :(
I see you are formatted themis sources and it's great anyway 👍

@ignatk
Copy link
Contributor Author

ignatk commented Dec 26, 2017

See PointerAlignment in https://clang.llvm.org/docs/ClangFormatStyleOptions.html

@ilammy
Copy link
Collaborator

ilammy commented Feb 28, 2019

🎉 Y—A—A—A—A—Y 🎉

Now the code style is updated and enforced for the core Soter and Themis libraries. Other C sources are still in disarray, but we'll sort that out. Eventually.

@gene-eu:

how long will it take to do this

Four days. For what could have been a simple clang fmt with no effort, if there was only one vendor of C compilers in the world... Well, at least we have this point covered for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Themis Core written in C, its packages enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants