-
Notifications
You must be signed in to change notification settings - Fork 94
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
[r2r] Get rid of fomat, wite macros #1316
Conversation
I think there is no reason to get rid of If we decide to replace our custom logger (as I described above) with the log crate at all, we won't be able to print emotions. |
* Remove unused code from `commmon.rs`
* Remove `for_c.rs`
3f71cdd
to
21b1ed4
Compare
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.
Thanks for the huge refactoring! I have few questions 🙂
mm2src/common/log.rs
Outdated
.unwrap(); | ||
$crate::log::chunk2log(buf, $crate::log::LogLevel::Info) | ||
let time = $crate::log::short_log_time($crate::now_ms()); | ||
let file = ::gstuff::filename (file!()); |
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.
Can we maybe also remove gstuff
usage here? 🙂
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.
It's used in many places. Do you suggest moving it to common
crate?
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.
@sergeyboyko0791 Possibly, or even use https://doc.rust-lang.org/stable/std/path/struct.Path.html#method.file_name if it matches our needs.
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 think gstuff::filename is much more efficient than Path::file_name, because it's based on two operations:
- Find last
/
or\
in the string slice - Checks if it's needed to return a slice without
.rs
suffix
My benchmarks:
gstuff::filename, 1000 iterations: 349.083µs
Path::file_name, 1000 iterations: 1.693791ms
gstuff::filename, 10000 iterations: 4.311166ms
Path::file_name, 10000 iterations: 19.391583ms
How about moving gstuff::filename
function to common.rs
?
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.
How about moving gstuff::filename function to common.rs?
Ok :)
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.
Done
* Use `log` macro on `mm2_main`
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.
Great work 🔥 !
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.
Re-approving.
commmon.rs
for_c.rs