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

style: add clang format #81

Merged
merged 2 commits into from
Apr 24, 2020
Merged

Conversation

gocarlos
Copy link
Member

@gocarlos gocarlos commented Apr 17, 2020

depends on #79

  1. commit here: add clang format and format script
  2. format source accordingly (automatically, no manual changes as impossible to review) -> this can be checked by getting first commit and running the format script -> should output the same as with the second commit applied

@coveralls
Copy link

coveralls commented Apr 17, 2020

Coverage Status

Coverage decreased (-5.8%) to 84.161% when pulling 6d7c246 on gocarlos:style--add-clang-format into d8a4ba0 on cose-wg:master.

src/cose_int.h Outdated Show resolved Hide resolved
src/cose_int.h Outdated Show resolved Hide resolved
@gocarlos
Copy link
Member Author

gocarlos commented Apr 17, 2020

Failing as coveralls is annoyingly going up and down with every change

@gocarlos
Copy link
Member Author

@jimsch please review this

@gocarlos
Copy link
Member Author

gocarlos commented Apr 17, 2020

I’d like to contribute some code, though it would make sense to format the code first

@jimsch
Copy link
Contributor

jimsch commented Apr 17, 2020

As you said style is subjective. I can live with most things but there are a couple of things that would drive me crazy:

  • Always use tabs to indent because different people think you should indent at different widths
  • In most cases I really want to have open braces NOT on a new line. Function declarations are the big exception to that rule.

In short I did a lot of my learning of style with K&R.

@jimsch
Copy link
Contributor

jimsch commented Apr 17, 2020

Part of the reason you are going to have a drop in coverage is that a large number of error cases don't have testing on them. This means that chaning

if (foo) deal with the error

to
if (foo) {
  deal with error
}
~~~
This increases the number of lines not covered.

@gocarlos
Copy link
Member Author

As you said style is subjective. I can live with most things but there are a couple of things that would drive me crazy:

  • Always use tabs to indent because different people think you should indent at different widths
  • In most cases I really want to have open braces NOT on a new line. Function declarations are the big exception to that rule.

In short I did a lot of my learning of style with K&R.

Sure, i’m 100% agnostic to the style.
Let me push a new version with this style

@gocarlos gocarlos force-pushed the style--add-clang-format branch 4 times, most recently from 41cc568 to dbff840 Compare April 18, 2020 09:42
@gocarlos
Copy link
Member Author

lets wait for #82 to land, otherwise we get tons of conflicts...

@gocarlos gocarlos force-pushed the style--add-clang-format branch 2 times, most recently from 29609e6 to 1bbfadd Compare April 19, 2020 08:07
@gocarlos
Copy link
Member Author

  • Always use tabs to indent because different people think you should indent at different widths

I have the feeling that spaces are the way to go with clang-format, when using tabs, the code looks pretty special (e.g. macros, function arguments etc.)...

It offers the option to costumize the width if using spaces, so not discussions needed here...
Also the default ones, LLVM, Chromium, Mozilla, Webkit etc. are all based on spaces, so I guess clang-format was tweaked for spaces...

If you really want tabs, I'd suggest that you remove this last commit and play with the clang-format file as well as with the reference here: https://clang.llvm.org/docs/ClangFormatStyleOptions.html

@jimsch
Copy link
Contributor

jimsch commented Apr 19, 2020

I can live with most of this, however some of it I really don't like.

  1. Please change to 4 character indents, this is why I use tabs not spaces because lots of people want different indent levels.
  2. I cannot find an option to control this, but I have found the following construct to be asking for errors
if (condition)
    statement;

vs
if (condition) {
    statement;
}

It is just too easy to add a second statement in the if statement w/o adding the braces. Is there a way to force doing braces in this case?
3. Add BeforeCatch: false and BeforeElse: false
4. Do you know what the line length is? I don't object to what is there I am just curious.

@gocarlos
Copy link
Member Author

Do you know what the line length is? I don't object to what is there I am just curious.

we are currently using chromium based style in general, this is based again on google style:
https://google.github.io/styleguide/cppguide.html#Line_Length which has default line length of 80 chars

@gocarlos
Copy link
Member Author

gocarlos commented Apr 20, 2020

2. I cannot find an option to control this, but I have found the following construct to be asking for errors

I'm not sure, if clang-format will be able to insert brakets to existing code, clang-format is mostly doing indentation, though we think exactly the same, and therefore we are using clang-tidy which reports on bad style: https://clang.llvm.org/extra/clang-tidy/checks/readability-braces-around-statements.html

this could be added in a next MR

@gocarlos
Copy link
Member Author

Do you know what the line length is? I don't object to what is there I am just curious.

we are currently using chromium based style in general, this is based again on google style:
https://google.github.io/styleguide/cppguide.html#Line_Length which has default line length of 80 chars

this can be seen by running ./scripts/build_clang_tidy.sh after applying #84

@jimsch
Copy link
Contributor

jimsch commented Apr 23, 2020

How soon do you think this is going to be ready to pull in. I am waiting on some stuff that I want to do for it.

@gocarlos
Copy link
Member Author

How soon do you think this is going to be ready to pull in. I am waiting on some stuff that I want to do for it.

This can be done at any time, i can rebase today and let you review again?

@jimsch
Copy link
Contributor

jimsch commented Apr 23, 2020

sounds good

@gocarlos gocarlos force-pushed the style--add-clang-format branch from 458e49f to 9b5349b Compare April 24, 2020 07:39
@gocarlos
Copy link
Member Author

sounds good

just realized that it would be very good to merge #93 first, as otherwise we get conflicts with cose_configure.h and cose_configure.h.in

@gocarlos
Copy link
Member Author

travis failing due to a timeout, nothing about this patch

@jimsch jimsch merged commit 9223f01 into cose-wg:master Apr 24, 2020
@gocarlos gocarlos deleted the style--add-clang-format branch April 24, 2020 16:30
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