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
+664 −0
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: 0 additions & 8 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,14 +41,6 @@ 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
9 changes: 7 additions & 2 deletions experimental/tinystr_neo/src/macros.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,13 @@
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: $crate::TinyAsciiStr<$n> =
$crate::TinyAsciiStr::from_str_panicky($s);
const TINYSTR_MACRO_CONST: $crate::TinyAsciiStr<$n> = {
match $crate::TinyAsciiStr::from_bytes($s.as_bytes()) {
Ok(s) => s,
// Cannot format the error since formatting isn't const yet
Err(_) => panic!(concat!("Failed to construct tinystr from ", $s)),
Copy link
Member

Choose a reason for hiding this comment

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

Praise: Good idea to use concat! to format a nicer message

}
};
TINYSTR_MACRO_CONST
}};
}
Expand Down