Message ID | 20240219163915.2705-1-dakr@redhat.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-71693-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:693c:2685:b0:108:e6aa:91d0 with SMTP id mn5csp1407840dyc; Mon, 19 Feb 2024 08:55:30 -0800 (PST) X-Forwarded-Encrypted: i=3; AJvYcCXH/twYK1TbHAQsXvO40QoIqymA97yJr3W+RwXVRxjNWaL89Xfl1+tSFlQe0fgjgLX2fdrpiobKz0Oo72F6eb9SOzWzPg== X-Google-Smtp-Source: AGHT+IGyS0UM/RgKelL3Wdi41RroLmI55Ui6Hyujwgbbko87e4RQEGA3saJhhKmad8nhVrQjEUK+ X-Received: by 2002:a05:6a00:3cd0:b0:6d9:b4a7:695a with SMTP id ln16-20020a056a003cd000b006d9b4a7695amr17694588pfb.16.1708361730008; Mon, 19 Feb 2024 08:55:30 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1708361729; cv=pass; d=google.com; s=arc-20160816; b=lp5j3dVVIfJCZw70xGHH89r6PfgnqLQs16ZMNiGBOeME7IiDdYHiFRAyrHwYvz20Xn ELdzZ5KAOQC3kO35T13Ob77FV0VA6Dj67v+fFAR0hMd7PTTXAoEcwdHpPcvOpBnH99rh to5KIUcIRvxmQh3on2YvqpwyieiWlIcE2BNb+DWTlSV6Eh5j7bWzf6ROm1v0NOuLvNUz Ww5FIlyIVNyDhhxoH3gil2qjXCkHA5ZAKdvAoi6l6+/3g4RT9jYbuuARWhuU9Bv8ukCE eIkpOB/ZdKvsoKhOtmAvRZyzv01Q+feC1IPAQZWqpam1Ki4TtRLCeevPU5qG12U1fQRW Pm8g== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:message-id:date:subject:cc:to :from:dkim-signature; bh=okXEMX2Wb0ZGER6a+MgWT/1TkbqrgHTCP6DrA/Jrr/Y=; fh=tIoCv9TjvKu0v7j4aWVSuUKkRKvfEPZO/ILZFfE1K60=; b=sE0f2ZpyxbJpDlVPbmxN9a31r4P2vt4doWxnHFOMBc+HZbm/Pp22dAa0edRF5Fv6vQ iXZwS7ToL1KUiSTdwN9qt0pm+bDC3ddDyWljadkCWwUjOMMmnoIj2haHx2w5FIyUqNsL qwkbO4gkabwKlMiKMNSIS3RU8jX3wQPjTZmIb+zDjM7d6KKtOCFGbrWaBkjC6w5BXeDL fTbISjNK1bfSNbfrNEjCItJrN7WDfF5KqWvIE4LnCGTh62TF8aczrx7H7/McqofhOM6f 1tzaaax7UZMV6xsSfzBVxEKWaIfOKbffT4f5DBucsk0/jdhzLs3F7w6P1iLXNs4+nhF5 Ci/A==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=exLjFMX4; arc=pass (i=1 spf=pass spfdomain=redhat.com dkim=pass dkdomain=redhat.com dmarc=pass fromdomain=redhat.com); spf=pass (google.com: domain of linux-kernel+bounces-71693-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) smtp.mailfrom="linux-kernel+bounces-71693-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from sy.mirrors.kernel.org (sy.mirrors.kernel.org. [147.75.48.161]) by mx.google.com with ESMTPS id a27-20020a630b5b000000b005dbc6dc4cdasi4792332pgl.255.2024.02.19.08.55.29 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 19 Feb 2024 08:55:29 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-71693-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161; Authentication-Results: mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=exLjFMX4; arc=pass (i=1 spf=pass spfdomain=redhat.com dkim=pass dkdomain=redhat.com dmarc=pass fromdomain=redhat.com); spf=pass (google.com: domain of linux-kernel+bounces-71693-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) smtp.mailfrom="linux-kernel+bounces-71693-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sy.mirrors.kernel.org (Postfix) with ESMTPS id 651ACB24594 for <ouuuleilei@gmail.com>; Mon, 19 Feb 2024 16:39:45 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 8CCBC40BF2; Mon, 19 Feb 2024 16:39:26 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="exLjFMX4" Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 63CAE36118 for <linux-kernel@vger.kernel.org>; Mon, 19 Feb 2024 16:39:21 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708360763; cv=none; b=Bad904ugyJIqw1Vf6u6sU3Gw3IwKoAEqYDHqL++U/43L5DaP+1q9Hk0Ue9cp10wX673YgW9obkjUFmRmErinOgUOw/1upxhFSLvFjps+ZBjMKLQlGxyZ65YxqHOKhkmxVOANU4e8Mj0bYaGopxhkQhlmq1E8ivsD0aM9ie4BWW0= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708360763; c=relaxed/simple; bh=C8EgG7XcdcMV2/sMB+b9Fci+Styb0h0r87nEyY8e7XM=; h=From:To:Cc:Subject:Date:Message-ID:MIME-Version; b=ereidtzTWSRHEknFh5qvfLGCDAsL7utSX6imKYUdaFhh17E8hfKZ6KsLEQ0B+ORXsxREjcEDFgc0j1nwfL68+idwbPCswrhT21+fr056nMCdz6voZzZtQrY9xthc8xnbrQBvayhFjhXywc6j8oAfxz85zXqsiu1ldFArssELYF4= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=exLjFMX4; arc=none smtp.client-ip=170.10.133.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1708360760; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding; bh=okXEMX2Wb0ZGER6a+MgWT/1TkbqrgHTCP6DrA/Jrr/Y=; b=exLjFMX4H44wepi4vZXz7S2AYKpWhezWI6R8YKznhq8I+Juf7pL2CbuKHhgunGURQtCvr3 n6vyMKW2+T6jNfpugHzhBZ6IR8PIsV7P6TkSUodcE3lykbU9istsoAtfTlgUllaJ1Y4F6t qoC83B2ckmXSKWvNoVC8EB0mwUkdMgE= Received: from mail-ej1-f71.google.com (mail-ej1-f71.google.com [209.85.218.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-374-tN3I47N4P3S3bntWvgCmtQ-1; Mon, 19 Feb 2024 11:39:19 -0500 X-MC-Unique: tN3I47N4P3S3bntWvgCmtQ-1 Received: by mail-ej1-f71.google.com with SMTP id a640c23a62f3a-a3e6bc4519fso75573466b.2 for <linux-kernel@vger.kernel.org>; Mon, 19 Feb 2024 08:39:18 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708360758; x=1708965558; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=okXEMX2Wb0ZGER6a+MgWT/1TkbqrgHTCP6DrA/Jrr/Y=; b=jqqk26o2TnHNEZAAcdCOM30UCKXHxB7Qe6Ej55XN7sQ+EF3N+uo85P67AmDPKDf6GL uhOpvKBn7CVh+D2TlWqGn0U8g8mQj7rtVfPFAv3hz84KVshd7/ZtMWDylfAADZkaTBSq tVrX1dcdvyX8fv8r1OJVg/a9BvwsyIzRixjb2qyTlwQHl4DGaXSbEWOVcwv8g1HRZXEe EG2Mf6IaqOPQMzVL8Q6gQ512sr0Zr2yC55o21dov4ZelvZY8ZXwDs9cb2+8/LBXsbGLR GtMp3khXweUcdkY0qer1w2m5qSm4ZZQ6SywClXijZ8bBBJSRs/wJBbTp2ddf45+v3vZw Gmgg== X-Forwarded-Encrypted: i=1; AJvYcCWe0artu1uLQRGPqh4ZVoQ62eUpBokcnXjSQHjO3PjusqgJr1a+Cyqeue1weIvexBBMQVq2+CvQrmRMO5DAsKi4KExQAucvPk2vWsg5 X-Gm-Message-State: AOJu0YyyyHqnWdK707aVZIHQajUoMSjMqPYeaBcRf09kTqsib4aAB5uY qs8+RLNd1nBBefG6YE3O2qcysA2jCnhZmEcmfcatlgSNTYI/P4QEqC7G/EX28Trct/yTLxH8yze r9AweLiQz+xGp91zX0nMJKNaR5vVPIk8/Obhi7HHEgzRiFro/+3SGfjJ9dSSDlw== X-Received: by 2002:a17:906:f288:b0:a3d:716e:820a with SMTP id gu8-20020a170906f28800b00a3d716e820amr10426723ejb.28.1708360757866; Mon, 19 Feb 2024 08:39:17 -0800 (PST) X-Received: by 2002:a17:906:f288:b0:a3d:716e:820a with SMTP id gu8-20020a170906f28800b00a3d716e820amr10426703ejb.28.1708360757516; Mon, 19 Feb 2024 08:39:17 -0800 (PST) Received: from cassiopeiae.. ([2a02:810d:4b3f:ee94:642:1aff:fe31:a19f]) by smtp.gmail.com with ESMTPSA id vu9-20020a170907a64900b00a3e59740f4esm2033586ejc.92.2024.02.19.08.39.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 19 Feb 2024 08:39:17 -0800 (PST) From: Danilo Krummrich <dakr@redhat.com> To: ojeda@kernel.org, alex.gaynor@gmail.com, wedsonaf@gmail.com, boqun.feng@gmail.com, gary@garyguo.net, bjorn3_gh@protonmail.com, benno.lossin@proton.me, a.hindborg@samsung.com, aliceryhl@google.com Cc: rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org, Danilo Krummrich <dakr@redhat.com> Subject: [PATCH v4] rust: str: add {make,to}_{upper,lower}case() to CString Date: Mon, 19 Feb 2024 17:39:13 +0100 Message-ID: <20240219163915.2705-1-dakr@redhat.com> X-Mailer: git-send-email 2.43.0 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1791347109416192608 X-GMAIL-MSGID: 1791347109416192608 |
Series |
[v4] rust: str: add {make,to}_{upper,lower}case() to CString
|
|
Commit Message
Danilo Krummrich
Feb. 19, 2024, 4:39 p.m. UTC
Add functions to convert a CString to upper- / lowercase, either
in-place or by creating a copy of the original CString.
Naming followes the one from the Rust stdlib, where functions starting
with 'to' create a copy and functions starting with 'make' perform an
in-place conversion.
This is required by the Nova project (GSP only Rust successor of
Nouveau) to convert stringified enum values (representing different GPU
chipsets) to strings in order to generate the corresponding firmware
paths. See also [1].
[1] https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/String.20manipulation.20in.20kernel.20Rust
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
Changes in V4:
- move to_ascii_{lower,upper}case() to CStr
- add a few comments suggested by Alice
Changes in V3:
- add an `impl DerefMut for CString`, such that these functions can be defined
for `CStr` as `&mut self` and still be called on a `CString`
Changes in V2:
- expand commit message mentioning the use case
- expand function doc comments to match the ones from Rust's stdlib
- rename to_* to make_* and add the actual to_* implementations
---
rust/kernel/str.rs | 87 +++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 86 insertions(+), 1 deletion(-)
base-commit: b401b621758e46812da61fa58a67c3fd8d91de0d
Comments
> Add functions to convert a CString to upper- / lowercase, either > in-place or by creating a copy of the original CString. > > Naming followes the one from the Rust stdlib, where functions starting > with 'to' create a copy and functions starting with 'make' perform an > in-place conversion. > > This is required by the Nova project (GSP only Rust successor of > Nouveau) to convert stringified enum values (representing different GPU > chipsets) to strings in order to generate the corresponding firmware > paths. See also [1]. > > [1] https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/String.20manipulation.20in.20kernel.20Rust > > Signed-off-by: Danilo Krummrich <dakr@redhat.com> This looks good to me, so you may add my Reviewed-by: Alice Ryhl <aliceryhl@google.com> However, it looks like this patch has some clippy warnings that need to be fixed before it can be merged. $ make LLVM=1 CLIPPY=1 error: unneeded `return` statement --> rust/kernel/str.rs:267:9 | 267 | return Ok(s); | ^^^^^^^^^^^^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_return = note: `-D clippy::needless-return` implied by `-D clippy::style` = help: to override `-D clippy::style` add `#[allow(clippy::needless_return)]` help: remove `return` | 267 - return Ok(s); 267 + Ok(s) | error: unneeded `return` statement --> rust/kernel/str.rs:284:9 | 284 | return Ok(s); | ^^^^^^^^^^^^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_return help: remove `return` | 284 - return Ok(s); 284 + Ok(s) | error: deref which would be done by auto-deref --> rust/kernel/str.rs:677:58 | 677 | unsafe { CStr::from_bytes_with_nul_unchecked_mut(&mut *self.buf) } | ^^^^^^^^^^^^^^ help: try: `&mut self.buf` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#explicit_auto_deref = note: `-D clippy::explicit-auto-deref` implied by `-D clippy::complexity` = help: to override `-D clippy::complexity` add `#[allow(clippy::explicit_auto_deref)]` error: aborting due to 3 previous errors Alice
On 2/20/24 10:35, Alice Ryhl wrote: >> Add functions to convert a CString to upper- / lowercase, either >> in-place or by creating a copy of the original CString. >> >> Naming followes the one from the Rust stdlib, where functions starting >> with 'to' create a copy and functions starting with 'make' perform an >> in-place conversion. >> >> This is required by the Nova project (GSP only Rust successor of >> Nouveau) to convert stringified enum values (representing different GPU >> chipsets) to strings in order to generate the corresponding firmware >> paths. See also [1]. >> >> [1] https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/String.20manipulation.20in.20kernel.20Rust >> >> Signed-off-by: Danilo Krummrich <dakr@redhat.com> > > This looks good to me, so you may add my > > Reviewed-by: Alice Ryhl <aliceryhl@google.com> Thanks for reviewing this patch. > > However, it looks like this patch has some clippy warnings that need to > be fixed before it can be merged. It seems that those warnings are treated as fatal even, although, given the rationale for these warnings, I'm not even sure those should be warnings at all. > > $ make LLVM=1 CLIPPY=1 > error: unneeded `return` statement I fail to see why being explicit is a bad thing in this context, and even more why this is treated as error. > --> rust/kernel/str.rs:267:9 > | > 267 | return Ok(s); > | ^^^^^^^^^^^^ > | > = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_return Following this link the given rationale is: "Removing the return and semicolon will make the code more rusty." That's the worst rationale I could think of. Without further rationale what that should mean and why this would be good, it's entirely meaningless. Instead, I'd argue that keeping it gives kernel people, who necessarily need to deal with both, Rust *and* C, more consistency in kernel code. At least, this shouldn't be fatal IMHO. > = note: `-D clippy::needless-return` implied by `-D clippy::style` > = help: to override `-D clippy::style` add `#[allow(clippy::needless_return)]` > help: remove `return` > | > 267 - return Ok(s); > 267 + Ok(s) > | > > error: unneeded `return` statement > --> rust/kernel/str.rs:284:9 > | > 284 | return Ok(s); > | ^^^^^^^^^^^^ > | > = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_return > help: remove `return` > | > 284 - return Ok(s); > 284 + Ok(s) > | > > error: deref which would be done by auto-deref Similar story here. Why is it bad, and even *fatal*, to be explicit? The answer from the link below: "This unnecessarily complicates the code." Again, not a great rationale, this is entirely subjective and might even depend on the context of the project. Again, for kernel people who need to deal with Rust *and* C continuously it might be better to be explicit. > --> rust/kernel/str.rs:677:58 > | > 677 | unsafe { CStr::from_bytes_with_nul_unchecked_mut(&mut *self.buf) } > | ^^^^^^^^^^^^^^ help: try: `&mut self.buf` > | > = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#explicit_auto_deref > = note: `-D clippy::explicit-auto-deref` implied by `-D clippy::complexity` > = help: to override `-D clippy::complexity` add `#[allow(clippy::explicit_auto_deref)]` > > error: aborting due to 3 previous errors > > Alice >
On Tue, Feb 20, 2024 at 1:02 PM Danilo Krummrich <dakr@redhat.com> wrote: > > On 2/20/24 10:35, Alice Ryhl wrote: > >> Add functions to convert a CString to upper- / lowercase, either > >> in-place or by creating a copy of the original CString. > >> > >> Naming followes the one from the Rust stdlib, where functions starting > >> with 'to' create a copy and functions starting with 'make' perform an > >> in-place conversion. > >> > >> This is required by the Nova project (GSP only Rust successor of > >> Nouveau) to convert stringified enum values (representing different GPU > >> chipsets) to strings in order to generate the corresponding firmware > >> paths. See also [1]. > >> > >> [1] https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/String.20manipulation.20in.20kernel.20Rust > >> > >> Signed-off-by: Danilo Krummrich <dakr@redhat.com> > > > > This looks good to me, so you may add my > > > > Reviewed-by: Alice Ryhl <aliceryhl@google.com> > > Thanks for reviewing this patch. > > > > > However, it looks like this patch has some clippy warnings that need to > > be fixed before it can be merged. > > It seems that those warnings are treated as fatal even, although, given the > rationale for these warnings, I'm not even sure those should be warnings at > all. The build currently succeeds with zero warnings. That's a very useful property, and I would not like to give it up. You could submit a patch to silence these specific warnings. The clippy::explicit_auto_deref warning is not one I care for, but I would object to silencing clippy::needless_return. Using return statements when you are not doing an early-return really is extremely unidiomatic Rust. Ultimately, I think there is a lot of value of just applying the code formatters and linters and following them to zero warnings. It ensures that the Rust code is relatively idiomatic, helps avoid long unproductive discussions, and makes it easy for me to fix formatting issues in my own patches (just run clippy and rustfmt on everything, and it lists only things that are my own fault). Alice
On 2/20/24 14:17, Alice Ryhl wrote: > On Tue, Feb 20, 2024 at 1:02 PM Danilo Krummrich <dakr@redhat.com> wrote: >> >> On 2/20/24 10:35, Alice Ryhl wrote: >>>> Add functions to convert a CString to upper- / lowercase, either >>>> in-place or by creating a copy of the original CString. >>>> >>>> Naming followes the one from the Rust stdlib, where functions starting >>>> with 'to' create a copy and functions starting with 'make' perform an >>>> in-place conversion. >>>> >>>> This is required by the Nova project (GSP only Rust successor of >>>> Nouveau) to convert stringified enum values (representing different GPU >>>> chipsets) to strings in order to generate the corresponding firmware >>>> paths. See also [1]. >>>> >>>> [1] https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/String.20manipulation.20in.20kernel.20Rust >>>> >>>> Signed-off-by: Danilo Krummrich <dakr@redhat.com> >>> >>> This looks good to me, so you may add my >>> >>> Reviewed-by: Alice Ryhl <aliceryhl@google.com> >> >> Thanks for reviewing this patch. >> >>> >>> However, it looks like this patch has some clippy warnings that need to >>> be fixed before it can be merged. >> >> It seems that those warnings are treated as fatal even, although, given the >> rationale for these warnings, I'm not even sure those should be warnings at >> all. > > The build currently succeeds with zero warnings. That's a very useful > property, and I would not like to give it up. Just to clarify, I did not say anything else. As mentioned, I think those should not even be warnings. > > You could submit a patch to silence these specific warnings. The I'm happy to do that. We should define the scope for that though. I think this should be set globally, or at least not per crate. However, I don't really know what's the best way to do that. We could pass '-Aclippy::' to the compiler... > clippy::explicit_auto_deref warning is not one I care for, but I would > object to silencing clippy::needless_return. Using return statements > when you are not doing an early-return really is extremely unidiomatic > Rust. Unfortunately, that's just a different version of: "Removing the return and semicolon will make the code more rusty." [1] Hence, I still fail to see why being explicit is a bad thing in this context. Is there any objective reason not to be allowed to be explicit here? > > Ultimately, I think there is a lot of value of just applying the code > formatters and linters and following them to zero warnings. It ensures > that the Rust code is relatively idiomatic, helps avoid long > unproductive discussions, and makes it easy for me to fix formatting > issues in my own patches (just run clippy and rustfmt on everything, > and it lists only things that are my own fault). Well, I generally agree. However, I'm clearly against *blindly* following formatters and linters. We should carefully pick the rules we follow, and we should do that for objective reasons. Forcing guidelines we don't have objective reasons for will otherwise just annoy people and lead to less ideal code for the project. And I intentionally say "for the project", since this context is important. [1] https://rust-lang.github.io/rust-clippy/master/index.html#/needless_return > > Alice >
On Tue, Feb 20, 2024 at 1:03 PM Danilo Krummrich <dakr@redhat.com> wrote: > > That's the worst rationale I could think of. Without further rationale what that > should mean and why this would be good, it's entirely meaningless. Probably whoever wrote that did not feel the need to explain further because it is the convention, but please feel free to open an issue/PR to Clippy about improving the wording of that text. The convention itself, however, you will find way harder to change everywhere else. > Instead, I'd argue that keeping it gives kernel people, who necessarily need to > deal with both, Rust *and* C, more consistency in kernel code. That sounds to me like trying to keep consistency in style/formatting between two languages, which is something we have discussed quite a few times in the past. We are keeping Rust code as idiomatic as possible, except where it may actually make sense to diverge for kernel reasons. But this one does not seem to be the case: - It is inconsistent with most Rust code out there. - It is inconsistent with all Rust kernel code. - It is inconsistent with learning material, which kernel developers use too. - It introduces 2 ways for writing the same trivial thing. - Rust is a more expression-oriented language than C. And, by the way, your patch does use both ways. Why aren't you explicit when it is a single expression too? > At least, this shouldn't be fatal IMHO. For some of the compiler-based (i.e. not Clippy) and that may make prototyping a bad experience, I could agree (e.g. like missing documentation is already a warning). But please note that patches must be warning free anyway, so it is not like this patch would have been OK. > Similar story here. Why is it bad, and even *fatal*, to be explicit? This one is more arguable, and could be discussed. In fact, we planned going through some of the lints in a meeting to see, mostly, what extra lints could be enabled etc. You are welcome to join if that happens (I think Trevor wanted to drive that discussion). > Again, not a great rationale, this is entirely subjective and might even depend > on the context of the project. Again, for kernel people who need to deal with Rust > *and* C continuously it might be better to be explicit. That is fine, but to decide on this like this, we need better examples and rationale than just "it might be better" (and please note that whatever Clippy says is not important, so complaining about their docs being lacking is not really an argument to change kernel code). Cheers, Miguel
On Tue, Feb 20, 2024 at 3:53 PM Danilo Krummrich <dakr@redhat.com> wrote: > > Just to clarify, I did not say anything else. As mentioned, I think those > should not even be warnings. Well, for things like the `return` one, I find it unlikely it will change. And for other things that there are 2 ways of doing it, what we typically want is to have one way of doing it, rather than allowing both ways. > I'm happy to do that. We should define the scope for that though. I think this > should be set globally, or at least not per crate. However, I don't really know > what's the best way to do that. We could pass '-Aclippy::' to the compiler... The scope is already defined -- it is global, precisely because we want to keep all kernel code as consistent as possible. > Is there any objective reason not to be allowed to be explicit here? What is not objective about wanting to be consistent? How is your argument objective if ours isn't? > Well, I generally agree. However, I'm clearly against *blindly* following > formatters and linters. > > Forcing guidelines we don't have objective reasons for will otherwise just annoy > people and lead to less ideal code for the project. And I intentionally say "for > the project", since this context is important. Who is *blindly* following formatters and linters? We don't have objective reasons? I don't appreciate the way you are wording things here. Does it actually lead to "less ideal code", or the opposite? Cheers, Miguel
On 2/20/24 16:04, Miguel Ojeda wrote: > On Tue, Feb 20, 2024 at 1:03 PM Danilo Krummrich <dakr@redhat.com> wrote: >> >> That's the worst rationale I could think of. Without further rationale what that >> should mean and why this would be good, it's entirely meaningless. > > Probably whoever wrote that did not feel the need to explain further > because it is the convention, but please feel free to open an issue/PR > to Clippy about improving the wording of that text. The rational for a convention can't be that it is a convention. Instead it should be a convention for an objective reason. > > The convention itself, however, you will find way harder to change > everywhere else. I'm not saying that we should enforce it otherwise, I just think that we should have objective reasons for restrictions. > >> Instead, I'd argue that keeping it gives kernel people, who necessarily need to >> deal with both, Rust *and* C, more consistency in kernel code. > > That sounds to me like trying to keep consistency in style/formatting > between two languages, which is something we have discussed quite a > few times in the past. No, I didn't say, nor did I mean, that we should align with C in general, nor should it be enforced. However, I also don't see why we should disallow it as long as there is no objective reason to do so. > > We are keeping Rust code as idiomatic as possible, except where it may > actually make sense to diverge for kernel reasons. > > But this one does not seem to be the case: > > - It is inconsistent with most Rust code out there. > - It is inconsistent with all Rust kernel code. > - It is inconsistent with learning material, which kernel developers use too.> - It introduces 2 ways for writing the same trivial thing. That's actually what the language did already with early-return vs return at the end of the function. I admit that consistent inconsistency is also kinda consistent though. :-) > - Rust is a more expression-oriented language than C. The language has various characteristics, maybe that's why it allows both? > > And, by the way, your patch does use both ways. Why aren't you > explicit when it is a single expression too? See above. > >> At least, this shouldn't be fatal IMHO. > > For some of the compiler-based (i.e. not Clippy) and that may make > prototyping a bad experience, I could agree (e.g. like missing > documentation is already a warning). > > But please note that patches must be warning free anyway, so it is not > like this patch would have been OK. Then it shouldn't be a warning either IMHO. > >> Similar story here. Why is it bad, and even *fatal*, to be explicit? > > This one is more arguable, and could be discussed. That's great, although I really don't understand why you think this one is, but the other one isn't. What's the difference? > In fact, we planned > going through some of the lints in a meeting to see, mostly, what > extra lints could be enabled etc. You are welcome to join if that > happens (I think Trevor wanted to drive that discussion). Thanks for the invitation, I'm happy to join! > >> Again, not a great rationale, this is entirely subjective and might even depend >> on the context of the project. Again, for kernel people who need to deal with Rust >> *and* C continuously it might be better to be explicit. > > That is fine, but to decide on this like this, we need better examples > and rationale than just "it might be better" (and please note that > whatever Clippy says is not important, so complaining about their docs > being lacking is not really an argument to change kernel code). I agree, but I also think it should be the other way around. We should have good examples and an objective rationale for things we restrict. > > Cheers, > Miguel >
On Tue, Feb 20, 2024 at 4:53 PM Danilo Krummrich <dakr@redhat.com> wrote: > > The rational for a convention can't be that it is a convention. Instead > it should be a convention for an objective reason. The rationale __for the lint__ is that it is a very established convention in Rust code. That is what Clippy is telling you. You may not agree with the convention (and thus the lint). That is fine, but it is still a fact that it is the convention, and that is why I said whoever wrote that Clippy description probably felt that wording is good enough. > I'm not saying that we should enforce it otherwise, I just think that we > should have objective reasons for restrictions. Again, you seem to be saying we do not have objective reasons. > However, I also don't see why we should disallow it as long as there is > no objective reason to do so. There are objective reasons to do so and we already explained them to you above: https://lore.kernel.org/rust-for-linux/CAH5fLggXiXGA_UWCxqqLhMpXe1kepDv2vMG+1jLGaDSzdRHvSw@mail.gmail.com/ https://lore.kernel.org/rust-for-linux/CANiq72nVkV3+1rt4Mi+Own6KGAzmvR2jf8fFsp9NBu_gy_ob5g@mail.gmail.com/ You disagree? Fine, if you can come up with an argument that convinces us or a good majority of the kernel that uses Rust, then I will happily apply the patch. Personally, I don't particularly care (based on style arguments alone) whether to write `return` or not. But I do care about not wasting time on litigating particular choices each time somebody does not like one. > That's actually what the language did already with early-return vs return at > the end of the function. > > I admit that consistent inconsistency is also kinda consistent though. :-) > > The language has various characteristics, maybe that's why it allows both? Languages may allow a lot of things, but that does not mean we write the code like IOCCC just because we can. Nor it does mean that a project should allow every way to write things either, _especially_ when it is a very widespread convention. > See above. I don't see it, sorry. You said you want to be "explicit", but one can very easily argue you are _not_ being explicit by omitting the `return` in some cases. > That's great, although I really don't understand why you think this one is, but > the other one isn't. What's the difference? We already told you in this thread several times -- the `return` one is idiomatic and so on. See the links above. > I agree, but I also think it should be the other way around. We should have good > examples and an objective rationale for things we restrict. Why? To me, we should have good examples and an objective rationale for things where we actually want to deviate from what everybody else is doing. Cheers, Miguel
On 2/20/24 16:45, Miguel Ojeda wrote: > On Tue, Feb 20, 2024 at 3:53 PM Danilo Krummrich <dakr@redhat.com> wrote: >> >> Just to clarify, I did not say anything else. As mentioned, I think those >> should not even be warnings. > > Well, for things like the `return` one, I find it unlikely it will change. > > And for other things that there are 2 ways of doing it, what we > typically want is to have one way of doing it, rather than allowing > both ways. > >> I'm happy to do that. We should define the scope for that though. I think this >> should be set globally, or at least not per crate. However, I don't really know >> what's the best way to do that. We could pass '-Aclippy::' to the compiler... > > The scope is already defined -- it is global, precisely because we > want to keep all kernel code as consistent as possible.> >> Is there any objective reason not to be allowed to be explicit here? > > What is not objective about wanting to be consistent? How is your > argument objective if ours isn't? Nothing, but I also never wrote that wanting to be consistent is not objective. I wrote that omitting the return statement is more "rusty" isn't an objective argument in my opinion. Maybe the misunderstanding is that consistent doesn't necessarily mean to do what everyone else does. Consistency has a scope. For instance, we can also be consistent with something that everyone else does differently, like linked lists throughout the kernel. > >> Well, I generally agree. However, I'm clearly against *blindly* following >> formatters and linters. >> >> Forcing guidelines we don't have objective reasons for will otherwise just annoy >> people and lead to less ideal code for the project. And I intentionally say "for >> the project", since this context is important. > > Who is *blindly* following formatters and linters? We don't have > objective reasons? This doesn't seem to be a "straight up" question. Hence, for the following I will assume that you feel like I said those things about you. First of all, I said that *we* (as a community) should not blindly follow formatters and linters and should have objective reasons for language restrictions in general. I never said anyone specific is doing that. Claiming that I did so, *would* be a straw man. Furthermore I was questioning two of the restrictions we already have and was asking for an objective rationale for those, since the clipply documentation does not provide an objective rationale in my opinion. Generally speaking, I don't think there is anything wrong in saying that I think some argument isn't objective (even if I'd be proven wrong). This is by far not the same as saying someone has no objective reasons (in general). This would be a straw man as well. > > I don't appreciate the way you are wording things here. I already had the feeling from your answer in the other thread. But, unfortunately, I don't understand why. If the above did not convince you otherwise, can you please explain? > > Does it actually lead to "less ideal code", or the opposite? > > Cheers, > Miguel >
On 2/20/24 17:53, Miguel Ojeda wrote: > On Tue, Feb 20, 2024 at 4:53 PM Danilo Krummrich <dakr@redhat.com> wrote: >> >> The rational for a convention can't be that it is a convention. Instead >> it should be a convention for an objective reason. > > The rationale __for the lint__ is that it is a very established > convention in Rust code. Understood. I'm basically just asking why this is the convention. Because I assume that there must be a good reason for that. If there is none, and it's really just because everyone is doing it, I personally think that's not an objective rationale. If there is no other reason it could even be just the opposite causality, as in it became the convention because clippy enforces it. (Disclaimer: I really don't know and so far I have no reason to assume so.) Generally, I don't think there is anything wrong with questioning things and I also don't think there is anything wrong in not accepting "because everyone does so" as an objective rationale. Otherwise I don't see any disagreement. I also understood that you want to be consistent and comply with this convention and surely I accept that. But again, I also think it's perfectly valid questioning things. > > That is what Clippy is telling you. > > You may not agree with the convention (and thus the lint). That is > fine, but it is still a fact that it is the convention, and that is > why I said whoever wrote that Clippy description probably felt that > wording is good enough. > >> I'm not saying that we should enforce it otherwise, I just think that we >> should have objective reasons for restrictions. > > Again, you seem to be saying we do not have objective reasons. I'm honestly sorry about this misunderstanding and that this seems to be an emotional discussion for you. I never said that you don't have objective reasons (in general). I just said that I don't consider *one specific rationale* as objective (or factual). And I think it's perfectly valid to claim the right to do so. This isn't a personal attack in any way.
diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index 7d848b83add4..4dec89455e2d 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -5,7 +5,7 @@ use alloc::alloc::AllocError; use alloc::vec::Vec; use core::fmt::{self, Write}; -use core::ops::{self, Deref, Index}; +use core::ops::{self, Deref, DerefMut, Index}; use crate::{ bindings, @@ -143,6 +143,19 @@ pub const fn from_bytes_with_nul(bytes: &[u8]) -> Result<&Self, CStrConvertError unsafe { core::mem::transmute(bytes) } } + /// Creates a mutable [`CStr`] from a `[u8]` without performing any + /// additional checks. + /// + /// # Safety + /// + /// `bytes` *must* end with a `NUL` byte, and should only have a single + /// `NUL` byte (or the string will be truncated). + #[inline] + pub unsafe fn from_bytes_with_nul_unchecked_mut(bytes: &mut [u8]) -> &mut CStr { + // SAFETY: Properties of `bytes` guaranteed by the safety precondition. + unsafe { &mut *(bytes as *mut [u8] as *mut CStr) } + } + /// Returns a C pointer to the string. #[inline] pub const fn as_char_ptr(&self) -> *const core::ffi::c_char { @@ -206,6 +219,70 @@ pub unsafe fn as_str_unchecked(&self) -> &str { pub fn to_cstring(&self) -> Result<CString, AllocError> { CString::try_from(self) } + + /// Converts this [`CStr`] to its ASCII lower case equivalent in-place. + /// + /// ASCII letters 'A' to 'Z' are mapped to 'a' to 'z', + /// but non-ASCII letters are unchanged. + /// + /// To return a new lowercased value without modifying the existing one, use + /// [`to_ascii_lowercase()`]. + /// + /// [`to_ascii_lowercase()`]: #method.to_ascii_lowercase + pub fn make_ascii_lowercase(&mut self) { + // INVARIANT: This doesn't introduce or remove NUL bytes in the C + // string. + self.0.make_ascii_lowercase(); + } + + /// Converts this [`CStr`] to its ASCII upper case equivalent in-place. + /// + /// ASCII letters 'a' to 'z' are mapped to 'A' to 'Z', + /// but non-ASCII letters are unchanged. + /// + /// To return a new uppercased value without modifying the existing one, use + /// [`to_ascii_uppercase()`]. + /// + /// [`to_ascii_uppercase()`]: #method.to_ascii_uppercase + pub fn make_ascii_uppercase(&mut self) { + // INVARIANT: This doesn't introduce or remove NUL bytes in the C + // string. + self.0.make_ascii_uppercase(); + } + + /// Returns a copy of this [`CString`] where each character is mapped to its + /// ASCII lower case equivalent. + /// + /// ASCII letters 'A' to 'Z' are mapped to 'a' to 'z', + /// but non-ASCII letters are unchanged. + /// + /// To lowercase the value in-place, use [`make_ascii_lowercase`]. + /// + /// [`make_ascii_lowercase`]: str::make_ascii_lowercase + pub fn to_ascii_lowercase(&self) -> Result<CString, AllocError> { + let mut s = self.to_cstring()?; + + s.make_ascii_lowercase(); + + return Ok(s); + } + + /// Returns a copy of this [`CString`] where each character is mapped to its + /// ASCII upper case equivalent. + /// + /// ASCII letters 'a' to 'z' are mapped to 'A' to 'Z', + /// but non-ASCII letters are unchanged. + /// + /// To uppercase the value in-place, use [`make_ascii_uppercase`]. + /// + /// [`make_ascii_uppercase`]: str::make_ascii_uppercase + pub fn to_ascii_uppercase(&self) -> Result<CString, AllocError> { + let mut s = self.to_cstring()?; + + s.make_ascii_uppercase(); + + return Ok(s); + } } impl fmt::Display for CStr { @@ -593,6 +670,14 @@ fn deref(&self) -> &Self::Target { } } +impl DerefMut for CString { + fn deref_mut(&mut self) -> &mut Self::Target { + // SAFETY: A `CString` is always NUL-terminated and contains no other + // NUL bytes. + unsafe { CStr::from_bytes_with_nul_unchecked_mut(&mut *self.buf) } + } +} + impl<'a> TryFrom<&'a CStr> for CString { type Error = AllocError;