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

Add .clang-format file #151

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add .clang-format file #151

wants to merge 6 commits into from

Conversation

jakelang
Copy link
Member

@jakelang jakelang commented Mar 13, 2018

Fixes #150

@chfast
Copy link
Collaborator

chfast commented Mar 14, 2018

Don't ever do this.

@chfast chfast closed this Mar 14, 2018
@axic axic removed the in progress label Mar 14, 2018
@jakelang
Copy link
Member Author

@chfast ?
was working on fixing it

@chfast
Copy link
Collaborator

chfast commented Mar 14, 2018

You can fix it, but reformatting all the code will never be accepted. You should propose .clang-format config first. I recommend the one from ethminer.

@chfast
Copy link
Collaborator

chfast commented Mar 14, 2018

Sorry I closed it prematurely.

@jakelang
Copy link
Member Author

ah ok. will roll back then. This format file is from cpp-ethereum

@axic axic reopened this Mar 14, 2018
@jakelang jakelang changed the title Run clang-format on sources Add .clang-format file Mar 14, 2018
@chfast
Copy link
Collaborator

chfast commented Mar 16, 2018

About fixing it: clang-format reorders the includes (if asked). This can be the reason for build errors.

@jakelang
Copy link
Member Author

Apparently this was not the issue, as I had tried earlier to simply switch around the only two includes.

@jakelang
Copy link
Member Author

jakelang commented May 11, 2018

This has not been touched in a long time. Do we still want to add a format file?
cc @chfast @axic

@chfast
Copy link
Collaborator

chfast commented May 11, 2018

I'm really for it. It can be different style, but it would make my life easier if I could reformat my changes with a single click.

@jakelang
Copy link
Member Author

What style would you prefer?

@chfast
Copy link
Collaborator

chfast commented May 11, 2018

@jakelang
Copy link
Member Author

jakelang commented May 11, 2018

Alright, I am using the ethminer clang-format file. I have no preference for the style outside of plain C.
@axic any thoughts about this?

@axic
Copy link
Member

axic commented Jul 29, 2018

I do not really like formatting changes like this:

-bool hasWasmPreamble(vector<uint8_t> const& _input) {
-  return
-    _input.size() >= 8 &&
-    _input[0] == 0 &&
-    _input[1] == 'a' &&
-    _input[2] == 's' &&
-    _input[3] == 'm' &&
-    _input[4] == 1 &&
-    _input[5] == 0 &&
-    _input[6] == 0 &&
-    _input[7] == 0;
+namespace
+{
+bool hasWasmPreamble(vector<uint8_t> const& _input)
+{
+    return _input.size() >= 8 && _input[0] == 0 && _input[1] == 'a' && _input[2] == 's' &&
+           _input[3] == 'm' && _input[4] == 1 && _input[5] == 0 && _input[6] == 0 && _input[7] == 0;
 }

The second one is way harder to follow. Is there a way to keep the former one?

@axic
Copy link
Member

axic commented Jul 29, 2018

I am all for having this merged, but couldn't so far find a config fulfilling all my requirements :)

@axic
Copy link
Member

axic commented Jul 29, 2018

Seems like the webkit style is the closest: https://webkit.org/code-style-guidelines/

Need to check these out:

BasedOnStyle (string)
The style used for all options not specifically set in the configuration.

This option is supported only in the clang-format configuration (both within -style='{...}' and the .clang-format file).

Possible values:

LLVM A style complying with the LLVM coding standards
Google A style complying with Google’s C++ style guide
Chromium A style complying with Chromium’s style guide
Mozilla A style complying with Mozilla’s style guide
WebKit A style complying with WebKit’s style guide

@chfast
Copy link
Collaborator

chfast commented Jul 30, 2018

If you want to experiment with it,

  • make sure you have the latest version of clang-format (6),
  • dump the config of the style you thinks is the closest:
clang-format-6.0 -style=WebKit -dump-config > .clang-format

. You will be able to see all available options.

@axic
Copy link
Member

axic commented Jul 30, 2018

Yeah I am reading the documentation and have a pretty good version now.

@chfast
Copy link
Collaborator

chfast commented Jul 30, 2018

I do not really like formatting changes like this:

-bool hasWasmPreamble(vector<uint8_t> const& _input) {

  • return
  • _input.size() >= 8 &&
  • _input[0] == 0 &&
  • _input[1] == 'a' &&
  • _input[2] == 's' &&
  • _input[3] == 'm' &&
  • _input[4] == 1 &&
  • _input[5] == 0 &&
  • _input[6] == 0 &&
  • _input[7] == 0;
    +namespace
    +{
    +bool hasWasmPreamble(vector<uint8_t> const& _input)
    +{
  • return _input.size() >= 8 && _input[0] == 0 && _input[1] == 'a' && _input[2] == 's' &&
  •       _input[3] == 'm' && _input[4] == 1 && _input[5] == 0 && _input[6] == 0 && _input[7] == 0;
    

}

I don't think there is any option to change how the expressions are packed. If you want hand-crafted expressions, wrap them with // clang-format off.

@axic axic self-assigned this Jul 30, 2018
@chfast
Copy link
Collaborator

chfast commented Aug 14, 2018

Can I use it for new code?

@axic axic added this to In progress in Public testnet (2nd milestone) via automation Dec 6, 2018
@jakelang
Copy link
Member Author

Rebased.

@codecov-io
Copy link

codecov-io commented Dec 11, 2018

Codecov Report

Merging #151 into master will increase coverage by 1.12%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #151      +/-   ##
==========================================
+ Coverage   50.89%   52.02%   +1.12%     
==========================================
  Files           8        8              
  Lines        1342     1311      -31     
  Branches      130      129       -1     
==========================================
- Hits          683      682       -1     
+ Misses        632      602      -30     
  Partials       27       27

@jakelang jakelang moved this from In progress to Needs review in Public testnet (2nd milestone) Jan 26, 2019
@axic axic force-pushed the clangformat branch 2 times, most recently from a7cb8c6 to eed78fb Compare February 13, 2019 13:29
@axic
Copy link
Member

axic commented Feb 13, 2019

@chfast please note I'll adjust the settings, just wanted to push CI first.

@chfast
Copy link
Collaborator

chfast commented Feb 13, 2019

I've noticed you touched this PR, so I dumped the style I'm currently using for Hera. I don't care which one is used in the end, but having one is big improvement for my daily workflow.

@axic axic added this to In progress in 0.2.3 via automation Feb 25, 2019
@chfast
Copy link
Collaborator

chfast commented Feb 26, 2019

What's the progress?

@axic axic removed this from In progress in 0.2.3 Apr 29, 2019
@axic axic added this to In progress in 0.2.4 via automation Apr 29, 2019
@axic axic force-pushed the clangformat branch 2 times, most recently from 6965e7d to 54505a8 Compare April 30, 2019 01:01
@axic
Copy link
Member

axic commented Apr 30, 2019

@chfast pushed some updates which are a bit closer to the current and Solidity's style. What do you think?

I still can't get over how ugly it formats wavm.cpp with those EEI definitions.

@chfast
Copy link
Collaborator

chfast commented Apr 30, 2019

I will take a look next week. I've notices clang-format has problems with formatting arrays of lambdas.

You don't want to format CMake files...

@axic
Copy link
Member

axic commented Apr 30, 2019

You don't want to format CMake files...

Yeah the final version of this PR won't do that.

@axic axic added this to In progress in 0.2.5 via automation May 22, 2019
@axic axic removed this from In progress in 0.2.4 May 22, 2019
@axic
Copy link
Member

axic commented Jun 9, 2020

Should copy the rules from https://github.com/wasmx/fizzy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
0.2.5
  
In progress
Development

Successfully merging this pull request may close these issues.

Setup clang-format
4 participants