Skip to content

apply coding style - #19

Merged
alex merged 1 commit into
Rust-for-Linux:rustfrom
kloenk:coding-style
Oct 5, 2020
Merged

alex merged 1 commit into
Rust-for-Linux:rustfrom
kloenk:coding-style

Conversation

@kloenk

@kloenk kloenk commented Oct 5, 2020

Copy link
Copy Markdown
Member

Add a rustfmt.toml config and run cargo fmt on the codebase.

Taken from Documentation/process/coding-style.rst

@kloenk

kloenk commented Oct 5, 2020

Copy link
Copy Markdown
Member Author

In my opinion needed for #1, as the kernel seems strict about those styles.

@kloenk

kloenk commented Oct 5, 2020

Copy link
Copy Markdown
Member Author

Do wee need the SPDX-License-Identifier: GPL-2.0-or-later header? we can check for the existence of that with rustfmt

@alex alex left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition, we should not be reformatting rust/shlex/ which is a 3p package.

Comment thread rustfmt.toml Outdated
@@ -0,0 +1,4 @@
max_width = 80
brace_style = "AlwaysNextLine"
tab_spaces = 8

ghost Oct 5, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to use a non-standard rustfmt.toml?

ghost Oct 5, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the coding style defined in Documentation/process/coding-style.rst

ghost Oct 5, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so the idea is that the Rust style should follow the C style? My intuition is that it'd be more valuable to follow the Rust ecosystem defaults -- the kernel needed it's own C style guide because there's no uniform C style. But Rust mostly has one, and we ought to follow that.

But I'm interested in the opinions of folks like @ojeda who have more upstream experience.

ghost Oct 5, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The coding style there was indeed intended for C. So I don't think we need to care too much about most of the rules in coding-style.rst. And it is valuable to take the chance to start fresh and follow standard Rust conventions as much as possible.

However, some points can/will be argued that they should be shared among all languages where possible:

  • The column width may be contentious. Linus recently allowed to go up to 100 if needed, so there is a precedent on moving these limits up (mainly backed on the fact that we have way bigger, better screens nowadays). On the other hand, I recently tried to raise .clang-format to 100 to help the tool a bit and the proposal was met with fair opposition since 80 is still supposed to be the limit.
  • The tab vs. space (and in particular size) is the bigger one and it also affects the column width. For scripts, it seems the choice is quite varied (e.g. there are Perl and Python scripts using tabs, 2 spaces, 4 spaces, spaces-for-half-indentation...), so there is precedent too. On the other hand, if Rust ends up being the first new language accepted into the kernel (i.e. for drivers and general code), then it is likely that formatting will be a discussion topic and I expect people to argue for tabs to be consistent with C. It is not unlikely the Cargo vs. Kbuild/direct-rustc that will likely take place for similar reasons.

Linus et. al. are flexible on style matters, so I think it will come down to what the majority of maintainers feel like it is the best option.

ghost Oct 5, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So Just keeping max_width = 100
And removing everything else?

ghost Oct 5, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good to me.

And thanks for looking at this!

ghost Oct 5, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me too!

ghost Oct 5, 2020 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, isn't max_width = 100 the default?

@ojeda

ghost commented Oct 5, 2020 •

Copy link
Copy Markdown
Member

Do wee need the SPDX-License-Identifier: GPL-2.0-or-later header? we can check for the existence of that with rustfmt

Yes, please, all files should have an SPDX ID (the ones that don't, like those in .github is because they won't be submitted).

Signed-off-by: Finn Behrens <me@kloenk.de>
@kloenk

ghost commented Oct 5, 2020

Copy link
Copy Markdown
Member Author

Do wee need the SPDX-License-Identifier: GPL-2.0-or-later header? we can check for the existence of that with rustfmt

Yes, please, all files should have an SPDX ID (the ones that don't, like those in .github is because they won't be submitted).

I just saw that that is not yet an stable feature of rustfmt. Do we still want the check for it?

@ojeda

ghost commented Oct 5, 2020

Copy link
Copy Markdown
Member

Do wee need the SPDX-License-Identifier: GPL-2.0-or-later header? we can check for the existence of that with rustfmt

Yes, please, all files should have an SPDX ID (the ones that don't, like those in .github is because they won't be submitted).

I just saw that that is not yet an stable feature of rustfmt. Do we still want the check for it?

Ah, I was answering the first question (i.e. we need the header). Concerning automatic checking, it'd be nice to enable it, but let's wait until it hits stable. I'd leave a comment to remind us to enable it when the time comes.

ghost left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think a good follow up task would be adding CI for this.

@alex
alex merged commit f60edb2 into Rust-for-Linux:rust Oct 5, 2020
@kloenk
kloenk deleted the coding-style branch October 6, 2020 03:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants