-
Notifications
You must be signed in to change notification settings - Fork 143
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.
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 complicated Maud benchmark #52
Add complicated Maud benchmark #52
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 on the general approach. Once these nits are fixed I'll be happy to merge.
@@ -64,7 +64,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" | |||
|
|||
[[package]] | |||
name = "libc" | |||
version = "0.2.16" | |||
version = "0.2.17" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These Cargo.lock
changes should go in a separate commit or be removed altogether -- it doesn't fit in with the rest of this patch
#[derive(Debug)] | ||
struct Entry { | ||
name: &'static str, | ||
score: u16, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this should be u32
(The original benchmarks were copied from Handlebars, which used u16
. In hindsight it would have been better to use u32
from the beginning.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opted for an usize since space is not an issue (and it will be aligned anyway) I thought that letting the compiler decide on the right thing (tm) might be the best
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going off rust-lang/rust#22240 which recommends u32
here.
It's not a performance/alignment issue, but a style one. From what I've seen, 32 bits is the "default" size, and what you should go for first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright! Sounds good, changed it
} | ||
} | ||
|
||
impl<'a> Display for Button<'a> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.with_method(RequestMethod::Post))) | ||
} | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There shouldn't be a ;
here. It's better to return the resulting Markup
from the closure, so that the compiler doesn't think it's unused and optimizes it out 😉
} | ||
} | ||
|
||
fn layout<S: AsRef<str>>(title: S, inner: Markup) -> Markup { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of AsRef<str>
here is pretty clever! I wonder if it's a good idea to apply in general.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've taken a liking to start using all the std::borrow
and std::convert
traits wherever I can, to leverage the 'zero-cost' in Rust (or I hope so) this is my favorite bit I wrote so far.
112ecad
to
2cbd848
Compare
Using |
Looks great, thanks! |
As part of #49 I've extracted a simple but still somewhat more involved example.
If you would like anything else to be added or more examples I could do that