linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Olliver Schinagl <oliver+list@schinagl.nl>
To: Gary Guo <gary@garyguo.net>
Cc: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>,
	Miguel Ojeda <ojeda@kernel.org>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	rust-for-linux <rust-for-linux@vger.kernel.org>,
	Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>,
	Linux Doc Mailing List <linux-doc@vger.kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 00/13] [RFC] Rust support
Date: Thu, 28 Jul 2022 22:43:26 +0200	[thread overview]
Message-ID: <a4803f2e-2e46-1c7d-0e89-96f5cbb0ad11@schinagl.nl> (raw)
In-Reply-To: <20220728112114.0000003a@garyguo.net>

Hey Gary,

On 28-07-2022 12:21, Gary Guo wrote:
> Hi Olliver,
> 
> On Wed, 27 Jul 2022 10:05:31 +0200
> Olliver Schinagl <oliver+list@schinagl.nl> wrote:
> 
>> Consitency is absolutly important! Zero argument there. My argument
>> is, the consistency should be within the kernel tree, not 'but the
>> rest of the world is using style X/Y/Z, lets be consistent with that.
>> In an utopia, maybe, but the real world doesn't work that way, sadly.
>> So in an attempt to standardize (rustfmt) they just "invented" a new
>> standard. Which btw is common, we see this happening every so often,
>> right?
> 
> Difference languages have different characteristics and I don't think
> it's necessarily good (and often isn't) to blindly apply coding style
> of one language onto another. So I don't see rustfmt as "inventing yet
> another standard" really, because there aren't many conflicting coding
> style standards in Rust world; almost everyone just settled on using
> rustfmt, mostly using the default, maybe with a few small
> project-specific configuration tweaks.
I was mostly arguing about a) lets look at this and b) having said 
configuration tweaks, rather then blindly (pun not really intended) 
going with rust's defaults :)

> 
> A small example for C and Rust differences:
> 
> Rust requires braces around branches of if expression, and C doesn't.
> So in kernel coding style you often have:
> 
> 	if (condition) do_something();
> 
> Or
> 
> 	if (condition)
> 		do_something();
> 
> But in Rust it will be:
> 
> 	if condition {
> 	    do_something();
> 	}
So kernel style kind of says 'no braces around single statements'; but 
if your rust compiler doesn't allow this; well then there's nothing to 
do. You could even argue to update the kernel C style on this to make it 
consistent again. BUT, this inconsistency makes it cognative 'hard'. If 
this if a C or a rust function? for example during a review. During 
authoring, when writing both C and rust code (due to nececity, not 
constant context switching) you cognitivly constantly have to go 
back/foward. While I'm sure there's people here that can do this all day 
without problem, some of of find this harder then needs to be. Hence the 
request to _try_ to keep consistency within the kernel tree.

> 
> That's just an example of one control flow constructions. There are
> differences between Rust match and C switch, etc. Rust's official
> coding style takes properties of Rust into consideration, so in many
> regards it's a more suitable coding style for Rust code in kernel, then
> applying kernel's C coding standard directly on kernel's Rust code.
> 
> Your earlier email in the thread also mentions about indentation, and I
> have a few things to point out as well.
> 
> First, Rust code typically requires more levels of indentation than C
> code. For example, many functions might be methods and they are inside
> an impl block, which creates one extra level of indentation.
> Statements inside match arms' block are two levels more indented than
> the match statement itself, as opposed to C's switch (as kernel coding
> style doesn't indent the case labels). As a result, 8 spaces for 1 level
> can be a bit excessive for Rust code, and thus the 4 space indentation
> used in rustfmt default.
> 
> Secondly, I don't think the argument about tabs being customisable
> holds; in kernel coding style tabs are strictly 8 characters. For line
Sure, this rule implies that for alignment, tabs should be set to 8 so 
things align nicely. However, nobody forces people to set their editor 
to 8 character width. Not doing so, doesn't break anything. At worst, 
you may commit something that is poorly aligned (but we _should_ be 
using tabs to indent, spaces to align anyway :p, tab == indent has meaning).

With non-tab indentation, this is no longer really possible, or at 
least, editors haven't solved that problem yet, as it tends to still 
break (due to the mixing of indentation and alignment using a single 
character). Maybe once we have AI and ML in our editors though :p

> continuation it's not uncommon to use a series of tabs followed by a
> few spaces, e.g.
> 
> 	int function_name(int first_argument,
> 	< tab  >< tab  >..int second_argument)
> 
> changing tab into 4 spaces will break the layout. (and I'll not go into
> well-known reasons about non-4-space-tab messing up code in terminal
> etc).
> 
>> Copy/pasting is known to cause bugs. There's actually research from
>> NASA on that. Code-reuse (libraries/functions) are not bad. But
>> (worst kind of example) copy paste from stack-overflow, or
>> copy/pasting stuff without actually looking at the content and
>> forgetting to rename something, causes bugs. Why is this relevant?
>> The whole 'lets be consistent with the rust codebase of the wrold'
>> argument. E.g. if everybody uses the same style (which is idealistic
>> and great) then copy/pasting becomes consistent. Where I say, try to
>> be careful when copy/pasting code.
> 
> When we vendor in code as a whole (e.g. like we currently do for
> alloc crate), it is proper code reuse. With different coding style the
> vendored code either diverges from upstream (which makes upstreaming
> much more difficult) or diverge from rest of kernel's Rust code base.
Very fair point of course. Though really, we should fix the upstream 
rust preferred format, but there it was already stated, that 'too bad, 
sorry' which from a developer point of view is fine, your project, your 
choice. From a disabilities point of view, sucks of course.

> 
>> But if that is the case, why not try to follow the kernels existing
>> code-style as close as possible with the rust-fmt configuration? I
>> know code-style has been discussed a few times over the decades; but
>> not many changes have been done, surely, if there's some codestyle
>> changes that are best for the kernel, they would have been 'advised'?
>> '4 space indents are better then 8-size tabs, on new code, try to use
>> them' for example :p
> 
> You do realize that you are creating a new coding style by doing this,
> right? It feels like creating problems rather than solving problems.
> 
> My personal feeling is that it's easier for me to adapt to different
> coding style when switching between languages, but it's rather awkward
> for me when trying to use different coding styles with the same
> language. I find myself no problem switching between 2 spaces when
> coding JavaScript to 4 spaces when coding Rust to 8 spaces(tab) when
> coding C, but it's rather painful to switch between C projects with
> different coding styles. I certainly don't want to switch between Rust
> projects with vastly different coding styles.
And I'm happy for you that you can easily take in 2 and 4 spaces. For 
me, it is extremly hard to read. So it's not a 'personal preference' 
thing. But I suggest to read the earlier posted links, where others at 
length explain it as well, how it is like to feel excluded becaues its 
just hard to read.

> 
>> But why? Why should we not be consistent with the kernels' code-base
>> (while yes, that is not rust, but C, but we can follow the same
>> style?)
> 
> Difference languages have different characteristics, and one size
> doesn't fit them all :)
I'm not even arguing this at all :)

I think the biggest issues i'm speaking of really are the braces and the 
spaces really, where the braces can be argued for/against, it's 
cognitive harder, but can be dealth with (and we can expect 
inconsitencies; but the sapces vs tabs thing, personal configuration vs 
forced with is the point I was trying to raise.

As said before, 'every building is different, some offer wheelchair 
ramps, others do' kind of point, not like 'this building is red, and 
that one is blue, and not every color fits all :p

> 
>> Sadly, I've seen so much vendor code (yeah, I know) which doesn't
>> even have consistency in their own files ...
> 
> That's very true. So when all other Rust code currently follow
> (roughly) the same coding style and this situation doesn't currently
> exist, let's not make it worse...
> 
> Best,
> Gary


  parent reply	other threads:[~2022-07-28 20:43 UTC|newest]

Thread overview: 201+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-14 18:45 ojeda
2021-04-14 18:45 ` [PATCH 01/13] kallsyms: Support "big" kernel symbols (2-byte lengths) ojeda
2021-04-14 19:44   ` Matthew Wilcox
2021-04-14 19:59     ` Miguel Ojeda
2021-04-14 18:45 ` [PATCH 02/13] kallsyms: Increase maximum kernel symbol length to 512 ojeda
2021-04-14 23:48   ` Nick Desaulniers
2021-04-14 18:45 ` [PATCH 03/13] Makefile: Generate CLANG_FLAGS even in GCC builds ojeda
2021-04-14 18:59   ` Nathan Chancellor
2021-04-15 10:18     ` Miguel Ojeda
2021-04-14 23:46   ` Nick Desaulniers
2021-04-15  0:47     ` Miguel Ojeda
2021-04-14 18:45 ` [PATCH 04/13] Kbuild: Rust support ojeda
2021-04-14 23:19   ` Nick Desaulniers
2021-04-15  0:43     ` Miguel Ojeda
2021-04-15 18:03       ` Nick Desaulniers
2021-04-16 12:23         ` Miguel Ojeda
2021-04-17 19:35       ` Masahiro Yamada
2021-04-16 13:38   ` Peter Zijlstra
2021-04-16 17:05     ` Linus Torvalds
2021-04-16 17:47       ` Miguel Ojeda
2021-04-16 18:09         ` Al Viro
2021-04-16 18:57           ` Miguel Ojeda
2021-04-16 20:22             ` Willy Tarreau
2021-04-16 20:34               ` Connor Kuehl
2021-04-16 20:58                 ` Willy Tarreau
2021-04-16 21:39                   ` Miguel Ojeda
2021-04-16 22:04                     ` Willy Tarreau
2021-04-16 22:45                       ` Al Viro
2021-04-16 23:46                       ` Miguel Ojeda
2021-04-17  4:24                         ` Willy Tarreau
2021-04-17 15:38                           ` Miguel Ojeda
2021-04-16 21:19               ` Miguel Ojeda
2021-04-16 17:34     ` Miguel Ojeda
2021-04-19 19:58       ` David Sterba
2021-04-19 20:17         ` Matthew Wilcox
2021-04-19 21:03           ` Miguel Ojeda
2021-04-19 20:54         ` Miguel Ojeda
2021-04-14 18:45 ` [PATCH 05/13] Rust: Compiler builtins crate ojeda
2021-04-14 19:19   ` Linus Torvalds
2021-04-14 19:34     ` Miguel Ojeda
2021-04-14 18:45 ` [PATCH 06/13] Rust: Module crate ojeda
2021-04-14 18:45 ` [PATCH 07/13] Rust: Kernel crate ojeda
2021-04-14 19:31   ` Linus Torvalds
2021-04-14 19:50     ` Miguel Ojeda
2021-04-14 18:45 ` [PATCH 08/13] Rust: Export generated symbols ojeda
2021-04-14 18:46 ` [PATCH 09/13] Samples: Rust examples ojeda
2021-04-14 19:34   ` Linus Torvalds
2021-04-14 19:42     ` Miguel Ojeda
2021-04-14 19:49       ` Matthew Wilcox
2021-04-16 11:46       ` Andrej Shadura
2021-04-14 23:24     ` Nick Desaulniers
2021-04-15  7:10       ` Greg Kroah-Hartman
2021-04-15  7:39         ` Nick Desaulniers
2021-04-15 12:42         ` Miguel Ojeda
2021-04-16 13:07         ` Sven Van Asbroeck
2021-04-16 13:20           ` Greg Kroah-Hartman
2021-04-14 18:46 ` [PATCH 10/13] Documentation: Rust general information ojeda
2021-04-14 22:17   ` Nick Desaulniers
2021-04-14 23:34     ` Miguel Ojeda
2021-04-14 18:46 ` [PATCH 11/13] MAINTAINERS: Rust ojeda
2021-04-14 21:55   ` Nick Desaulniers
2021-04-14 22:02     ` Miguel Ojeda
2021-04-14 22:36   ` Nick Desaulniers
2021-04-14 18:46 ` [PATCH 12/13] Rust: add abstractions for Binder (WIP) ojeda
2021-04-14 18:46 ` [PATCH 13/13] Android: Binder IPC in Rust (WIP) ojeda
2021-04-14 19:44 ` [PATCH 00/13] [RFC] Rust support Linus Torvalds
2021-04-14 20:20   ` Miguel Ojeda
2021-04-15  1:38     ` Kees Cook
2021-04-15  8:26       ` David Laight
2021-04-15 18:08         ` Kees Cook
2021-04-15 12:39       ` Miguel Ojeda
2021-04-14 20:09 ` Matthew Wilcox
2021-04-14 20:21   ` Linus Torvalds
2021-04-14 20:35     ` Josh Triplett
2021-04-14 22:08     ` David Laight
2021-04-14 20:29   ` Miguel Ojeda
2021-04-18 15:31   ` Wedson Almeida Filho
2021-04-15  0:22 ` Nick Desaulniers
2021-04-15 10:05   ` Miguel Ojeda
2021-04-15 18:58 ` Peter Zijlstra
2021-04-16  2:22   ` Wedson Almeida Filho
2021-04-16  4:25     ` Al Viro
2021-04-16  5:02       ` Wedson Almeida Filho
2021-04-16  5:39         ` Paul Zimmerman
2021-04-16  7:46         ` Peter Zijlstra
2021-04-16  7:09     ` Peter Zijlstra
2021-04-17  5:23       ` comex
2021-04-17 12:46       ` David Laight
2021-04-17 14:51       ` Paolo Bonzini
2021-04-19  7:32         ` Peter Zijlstra
2021-04-19  7:53           ` Paolo Bonzini
2021-04-19  8:26             ` Peter Zijlstra
2021-04-19  8:35               ` Peter Zijlstra
2021-04-19  9:02               ` Paolo Bonzini
2021-04-19  9:36                 ` Peter Zijlstra
2021-04-19  9:40                   ` Paolo Bonzini
2021-04-19 11:01                     ` Will Deacon
2021-04-19 17:14                   ` Linus Torvalds
2021-04-19 18:38                     ` Paolo Bonzini
2021-04-19 18:50                       ` Linus Torvalds
2021-04-22 10:03     ` Linus Walleij
2021-04-22 14:09       ` David Laight
2021-04-22 15:24       ` Wedson Almeida Filho
2021-04-26  0:18         ` Linus Walleij
2021-04-26 14:26           ` Miguel Ojeda
2021-04-26 14:40           ` Wedson Almeida Filho
2021-04-26 16:03             ` Miguel Ojeda
2021-04-27 10:54             ` Linus Walleij
2021-04-27 11:13               ` Robin Randhawa
2021-04-29  1:52               ` Wedson Almeida Filho
2021-04-26 18:01           ` Miguel Ojeda
2021-04-22 21:28       ` Miguel Ojeda
2021-04-26  0:31         ` Linus Walleij
2021-04-26 18:18           ` Miguel Ojeda
2021-04-27 11:13             ` Linus Walleij
2021-04-28  2:51               ` Kyle Strand
2021-04-28  3:10               ` Miguel Ojeda
2021-05-04 21:21                 ` Linus Walleij
2021-05-04 23:30                   ` Miguel Ojeda
2021-05-05 11:34                     ` Linus Walleij
2021-05-05 14:17                       ` Miguel Ojeda
2021-05-05 15:13                         ` Enrico Weigelt, metux IT consult
2021-05-06 12:47                         ` Linus Walleij
2021-05-07 18:23                           ` Miguel Ojeda
2021-04-16  4:27   ` Boqun Feng
2021-04-16  6:04     ` Nick Desaulniers
2021-04-16 18:47       ` Paul E. McKenney
2021-04-19 20:35         ` Nick Desaulniers
2021-04-19 21:37           ` Paul E. McKenney
2021-04-19 22:03           ` Miguel Ojeda
2021-04-16 20:48     ` Josh Triplett
2021-04-16  8:16   ` Michal Kubecek
2021-04-16  9:29     ` Willy Tarreau
2021-04-16 11:24 ` Peter Zijlstra
2021-04-16 13:07   ` Wedson Almeida Filho
2021-04-16 14:19     ` Peter Zijlstra
2021-04-16 15:04       ` Miguel Ojeda
2021-04-16 15:43         ` Peter Zijlstra
2021-04-16 16:21           ` Miguel Ojeda
2021-04-16 15:33       ` Wedson Almeida Filho
2021-04-16 16:14         ` Willy Tarreau
2021-04-16 17:10           ` Miguel Ojeda
2021-04-16 17:18             ` Peter Zijlstra
2021-04-16 18:08               ` Matthew Wilcox
2021-04-17 11:17                 ` Peter Zijlstra
2021-04-17 11:46                   ` Willy Tarreau
2021-04-17 14:24                     ` Peter Zijlstra
2021-04-17 14:36                       ` Willy Tarreau
2021-04-17 13:46                   ` David Laight
2021-04-16 17:37             ` Willy Tarreau
2021-04-16 17:46               ` Connor Kuehl
2021-04-20  0:24               ` Nick Desaulniers
2021-04-20  3:47                 ` Willy Tarreau
2021-04-20  5:56                 ` Greg Kroah-Hartman
2021-04-20  6:16                   ` Willy Tarreau
2021-04-29 15:38                     ` peter enderborg
2021-04-17 13:53           ` Wedson Almeida Filho
2021-04-17 14:21             ` Willy Tarreau
2021-04-17 15:23               ` Miguel Ojeda
2021-04-18 15:51               ` Wedson Almeida Filho
2021-04-17 12:41       ` David Laight
2021-04-17 13:01         ` Wedson Almeida Filho
2021-04-16 15:03     ` Matthew Wilcox
2021-04-17 13:29       ` Wedson Almeida Filho
2021-04-16 15:58     ` Theodore Ts'o
2021-04-16 16:21       ` Wedson Almeida Filho
2021-04-17 15:11       ` Paolo Bonzini
2021-04-16 14:21   ` Miguel Ojeda
2021-04-17 20:42 ` Richard Weinberger
2021-04-28 18:34 ` Mariusz Ceier
2021-04-28 20:25   ` Nick Desaulniers
2021-04-28 21:21   ` David Laight
2021-04-29 11:14     ` Kajetan Puchalski
2021-04-29 11:25   ` Kajetan Puchalski
2021-04-29 14:06     ` Mariusz Ceier
2021-04-29 14:13       ` Sven Van Asbroeck
2021-04-29 14:26         ` Willy Tarreau
2021-04-29 15:06       ` Al Viro
2021-04-29 16:09         ` Mariusz Ceier
2021-04-30  6:39     ` Thomas Schoebel-Theuer
2021-04-30  8:30       ` David Laight
2021-05-05 13:58       ` Enrico Weigelt, metux IT consult
2021-05-05 14:41         ` Miguel Ojeda
2022-06-20 15:11 ` Olliver Schinagl
2022-06-27 17:44   ` Miguel Ojeda
2022-07-18  6:56     ` Olliver Schinagl
2022-07-20 19:23       ` Miguel Ojeda
2022-07-20 20:21         ` Nicolas Pitre
2022-07-27  7:47           ` Olliver Schinagl
2022-07-27 13:32             ` Nicolas Pitre
2022-07-27  8:05         ` Olliver Schinagl
2022-07-28 10:21           ` Gary Guo
2022-07-28 12:09             ` Greg Kroah-Hartman
2022-07-28 12:28               ` Gary Guo
2022-07-28 20:45               ` Olliver Schinagl
2022-07-29  8:04                 ` Greg Kroah-Hartman
2022-07-28 20:43             ` Olliver Schinagl [this message]
2021-04-29  5:20 Mariusz Ceier
2021-04-29  5:21 Mariusz Ceier
2021-04-29  8:18 ` David Laight
2021-07-30 23:22 Dillan Jackson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a4803f2e-2e46-1c7d-0e89-96f5cbb0ad11@schinagl.nl \
    --to=oliver+list@schinagl.nl \
    --cc=gary@garyguo.net \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kbuild@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=miguel.ojeda.sandonis@gmail.com \
    --cc=ojeda@kernel.org \
    --cc=rust-for-linux@vger.kernel.org \
    --cc=torvalds@linux-foundation.org \
    --subject='Re: [PATCH 00/13] [RFC] Rust support' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).