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.

Sign up for GitHub

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 tinystr-neo to experimental/ #1508

Merged
merged 12 commits into from
Jan 18, 2022
Merged

Add tinystr-neo to experimental/ #1508

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
8c13b98
Add tinystr_neo experimental crate
Manishearth Jan 14, 2022
ad88484
Basic TinyAsciiStr type
Manishearth Jan 14, 2022
3044449
Add serde impls
Manishearth Jan 14, 2022
1361678
Add tests for serde
Manishearth Jan 14, 2022
510c57a
fix var name
Manishearth Jan 14, 2022
73a799d
rm bytes
Manishearth Jan 16, 2022
6bfbfc3
fmt
Manishearth Jan 16, 2022
2cdeab3
tidy
Manishearth Jan 16, 2022
347c955
Make macro panic at compile time
Manishearth Jan 17, 2022
601787d
clippy
Manishearth Jan 17, 2022
d009c19
move panic
Manishearth Jan 17, 2022
2d9e801
clip
Manishearth Jan 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions experimental/tinystr_neo/src/ascii.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,14 @@ impl<const N: usize> TinyAsciiStr<N> {
Self::from_bytes(s.as_bytes())
}

pub const fn from_str_panicky(s: &str) -> Self {
match Self::from_bytes(s.as_bytes()) {
Ok(s) => s,
// Cannot format the error since formatting isn't const yet
Err(_) => panic!("Failed to construct tinystr"),
}
}

pub fn len(&self) -> usize {
self.bytes.iter().position(|x| *x == 0).unwrap_or(N)
Copy link
Member Author

Choose a reason for hiding this comment

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

note: previously we were able to use .trailing_zeroes()/leading_zeroes() which are a single instruction. If rustc unrolls this loop for small N we should also be fine here, but I'm not sure if that's happening, we should bench this.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah; this is one of the things I was wondering how we would implement efficiently. We should definitely test. I think this would be a good thing for IAI as opposed to Clippy.

}
Expand Down
14 changes: 6 additions & 8 deletions experimental/tinystr_neo/src/macros.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,12 @@

#[macro_export]
macro_rules! tinystr {
Copy link
Member

Choose a reason for hiding this comment

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

Thought: We maybe don't need the macro any more. People can just use the const function constructors directly. Fewer imports and friendlier to the compiler.

Copy link
Member Author

Choose a reason for hiding this comment

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

So as mentioned in #1511 (comment) , I think it's important to give users a convenient way to call this in a const context, given that it's rare and unusual for Rust programmers to const locals wherever possible. Unlike that PR, there will be no exported consts for people to use this, so I'd prefer to keep the macro around.

MBEs (non proc macros, like this one) are pretty friendly to the compiler; and this is going to be even smaller once I rebase over the rust update. I don't find that argument that compelling.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, macro_rules is definitely an improvement over a proc macro. Okay, I'm convinced.

($n:literal, $s:literal) => {
({
// Force it into a const context; otherwise it may get evaluated at runtime instead.
const TINYSTR_MACRO_CONST: Result<$crate::TinyAsciiStr<$n>, $crate::TinyStrError> = $crate::TinyAsciiStr::from_str($s);
TINYSTR_MACRO_CONST
// Note: Rust 1.57 allows panics in consts, until we update to that version we have to panic outside of the const and hope it optimizes away
}).unwrap()
}
($n:literal, $s:literal) => {{
// Force it into a const context; otherwise it may get evaluated at runtime instead.
const TINYSTR_MACRO_CONST: $crate::TinyAsciiStr<$n> =
$crate::TinyAsciiStr::from_str_panicky($s);
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: does it work if you do this?

Suggested change
const TINYSTR_MACRO_CONST: $crate::TinyAsciiStr<$n> =
$crate::TinyAsciiStr::from_str_panicky($s);
const TINYSTR_MACRO_CONST: $crate::TinyAsciiStr<$n> =
$crate::TinyAsciiStr::from_str($s).unwrap();

or perhaps

Suggested change
const TINYSTR_MACRO_CONST: $crate::TinyAsciiStr<$n> =
$crate::TinyAsciiStr::from_str_panicky($s);
const TINYSTR_MACRO_CONST: $crate::TinyAsciiStr<$n> =
$crate::TinyAsciiStr::from_str($s).ok().expect("Invalid tinystr");

Copy link
Member

Choose a reason for hiding this comment

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

(with the unwrap inside the const context)

Copy link
Member Author

@Manishearth Manishearth Jan 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

No, unwrap isn't stably const

Copy link
Member

Choose a reason for hiding this comment

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

Not even Option::unwrap? oh well... how about

const TINYSTR_MACRO_CONST: $crate::TinyAsciiStr<$n> =
            match $crate::TinyAsciiStr::from_str($s) {
    Ok(s) => s,
    Err(_) => panic!("{}", $s)  // Invalid tinystr
}

Or, keep it the way it is. It's just that it might be nice to not have a panicky library function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I didn't like that function either, moved

TINYSTR_MACRO_CONST
}};
}

#[cfg(test)]
Expand Down