rust: kernel: move the trim_whitespace() from sysctl to str

[qidu]

As there is already a str, there put the trim_whitespace() into str.rs would be better.

[ojeda]

Moving it to str.rs is fine, but should it be inside CStr?

[qidu]

Moving it to str.rs is fine, but should it be inside CStr?

No, it's not necessary inside CStr. I could move it outside.

[Skgland]

This might be out-of-scope for this PR, which just moves code as is,
but maybe it would also be a bit nicer to use while let.
That way one could combine the is empty test with the whitespace test and the indexing for the remainder.
Edit: And without the manual indexing this can also be const.

#[inline]
pub const fn trim_whitespace(mut data: &[u8]) -> &[u8] {
    while let [b' ' | b'\t' | b'\n', remainder @ ..] = data {
        data = remainder;
    }
    while let [remainder @ .., b' ' | b'\t' | b'\n'] = data {
        data = remainder;
    }
    data
}

#[test]
fn test_trim_whitespace() {
    assert_eq!(trim_whitespace(b""       ), b"");
    assert_eq!(trim_whitespace(b"       "), b"");
    assert_eq!(trim_whitespace(b"foo    "), b"foo");
    assert_eq!(trim_whitespace(b"    foo"), b"foo");
    assert_eq!(trim_whitespace(b"  foo  "), b"foo");
}

playground
godbolt

[qidu]

Thanks for your advices!

This might be out-of-scope for this PR, which just moves code as is, but maybe it would also be a bit nicer to use while let. That way one could combine the is empty test with the whitespace test and the indexing for the remainder. Edit: And without the manual indexing this can also be const.

[ojeda]

@Skgland Yeah, I like the @ matching, the extra tests and the const and the #[inline]. Thanks a lot for taking the time to check in CE! The diff in CE appears to be equivalent: https://godbolt.org/z/Ex5dT8ffr

@qidu Please rebase your commits so that one commit does the move as-is and another changes the while loop (you can take the chance to explain the codegen is the same, e.g. linking to Compiler Explorer), the extra tests, etc.

Also please read Documentation/process/submitting-patches.rst. In particular, you will need to reword your commit messages to explain why each patch is needed, and also sign your work with your real name to certify you that you wrote it or otherwise have the right to pass it on as an open-source patch (Developer's Certificate of Origin).

And, of course, you will need to add docs if you plan to make it pub and pass the CI.

[ojeda]

By the way, I would give credit to @Skgland for the second part, or even better, he could submit it himself after you perform the move.

[Skgland]

By the way, I would give credit to @Skgland for the second part, or even better, he could submit it himself after you perform the move.

Yeah, I could do that as well. Though I am fine either way.

@Skgland Yeah, I like the @ matching, the extra tests and the const and the #[inline]. Thanks a lot for taking the time to check in CE! The diff in CE appears to be equivalent: https://godbolt.org/z/Ex5dT8ffr

Ah, I was unable to find the diff view and compared them manually.
Though the #[inline] was added in @qidu's first commit (4f6559f) moving the function into CStr and not by myself.

[Skgland]

Do I understand Documentation/process/submitting-patches.rst correctly in that my changes when submitted as by @qidu should also contain my Signed-off-by?

The Signed-off-by: tag indicates that the signer was involved in the development of the patch, or that he/she was in the patch's delivery path.

If that the case than that would be:

Signed-off-by: Bennet Bleßmann <[email protected]>

[ojeda]

Do I understand Documentation/process/submitting-patches.rst correctly in that my changes when submitted as by @qidu should also contain my Signed-off-by?

Almost -- that tag should be there, as well as the From: you, but if you want to go as co-authors, then also a Co-developed-by one by him needs to be there, like this:

From: Bennet Bleßmann <[email protected]>

<changelog>

Signed-off-by: Bennet Bleßmann <[email protected]>
Co-developed-by: @qidu real name <email>
Signed-off-by: @qidu real name <email>

But if you are happy sending it yourself, then you will both learn how to prepare commits for the kernel :)

[ojeda]

Though the #[inline] was added in @qidu's first commit (4f6559f) moving the function into CStr and not by myself.

Ah, sorry.

Actually, it is not a small function, so we probably don't want it #[inline]. @qidu: was there a reason for the change?

[qidu]

@ojeda Thanks for all your advices.
@Skgland Please go on adding your while loop.

[nbdd0121]

We have a patch that'll make BStr a new-type wrapper instead of a type alias (#893). This trim function could then instead be made as a method of BStr.

[Skgland]

I submitted PR #913 with my changes, as this is not yet merged it currently also shows @qidu's commit over there.

[ojeda]

Please rebase the branch to create proper commits -- I would suggest checking other PRs, for instance https://github.com/Rust-for-Linux/linux/pull/910/commits.

Thanks!

[ojeda]

Later on (in another PR, likely after #913) we may want to consider trimming \v\f\r too (and adjust the docs accordingly to say ASCII whitespace instead).

[ojeda]

Please rebase with fixup the new commits into the first. Also for the commit message, you may add that you made the function public and that you moved the tests into an example etc.

[tomilepp]

Should we add tests for \n and \t too?

assert_eq!(trim_whitespace(b"\nfoo\t "), b"foo");

and test for non stripping

assert_eq!(trim_whitespace(b"f o\no\tbar "), b"f o\no\tbar");