Code cleanup and maintenance #85
Conversation
Signed-off-by: Alex Saveau <[email protected]>
There was a problem hiding this comment.
There are some changes that are quite nice, but also some that I would definitely want some elaboration on why the change should be made. I annotated places like these with inline comments. I want to stress that my requests for motivation are by no means a rejection or a push-back against the change in question; rather I just think it would be a good idea to have some text written down to make reasoning explicit (even if it is “just because”) and to reduce the space for assumptions.
As for let-else, I suspect it’ll be difficult to convince me of its benefit in the uses presented, but I’m happy to debate and to be shown errors of my ways ;)
tracing-tracy/Cargo.toml
Outdated
| @@ -3,7 +3,7 @@ name = "tracing-tracy" | |||
| version = "0.10.4" | |||
| authors = ["Simonas Kazlauskas <[email protected]>"] | |||
| license = "MIT/Apache-2.0" | |||
| edition = "2018" | |||
| edition = "2021" | |||
There was a problem hiding this comment.
Is there a reason why you think this change is something we should do?
There was a problem hiding this comment.
No, this is more of a "we should be up-to-date" axium. The changes are listed in this chapter: https://doc.rust-lang.org/edition-guide/rust-2021/index.html.
Why not? is probably a better question IMO.
| @@ -51,18 +51,18 @@ pub mod internal { | |||
| use std::ffi::CString; | |||
| pub use std::ptr::null; | |||
|
|
|||
| #[inline(always)] | |||
| #[must_use] | |||
There was a problem hiding this comment.
Unless there is a good reason not to (e.g. measured performance is worse,) lets keep the #[inline(always)].
There was a problem hiding this comment.
Did you benchmark their use? The default should be to not use inline(always): https://rust-lang.github.io/rust-clippy/master/index.html#/inline_always. I've also had folks from the stdlib team tell me that it slows down compile times and bloats codegen if llvm thinks using a function would actually be better. Basically the rule is that you need to have measured a perf improvement before using inline(always).
There was a problem hiding this comment.
I do recall looking at the assembly way back for some of these. It might very well be the case that after the many LLVM upgrades the situation has changed, or it might be that it hasn’t, I don’t really have the time to look into this right now.
That said, I can motivate why these should be #[inline(always)] even outside of the “measure-and-prove” line of reasoning.
Particularly worth noting is that many of these internal functions are just basic wrappers over otherwise private constructors to be used in this crate’s macros. Constructing an ADT is pretty much always simpler than making a function call to do the same, no matter how many uses of this operation there are.
This is especially notable in the context of the compile-time blowup reasoning that clippy presents as a counter-argument. It makes a lot of sense when we’re talking about functions with a non-trivial amount of logic and such. It does not hold at all for wrapper functions like these (and clippy itself presents some exceptions such as functions that have nothing in their body or simply panic.)
If it wasn’t for the limitations in the Rust’s visibility system in combination with macros, these functions wouldn’t exist at all. In that sense #[inline(always)] serves as a pretty good mechanism to also express that sort of intent.
Finally inline(always) has a benefit of achieving these intents even in debug builds.
There was a problem hiding this comment.
I think that makes sense? It still leaves me uneasy lol. Anyway, I brought back the inline(always) attributes for the mod internal methods.
Did you want all the inlines brought back? I'll disagree a bit more strongly about the others.
Signed-off-by: Alex Saveau <[email protected]>
Signed-off-by: Alex Saveau <[email protected]>
Signed-off-by: Alex Saveau <[email protected]>
Signed-off-by: Alex Saveau <[email protected]>
…locals: #86 Signed-off-by: Alex Saveau <[email protected]>
Signed-off-by: Alex Saveau <[email protected]>
|
Alrighty! Unless there's more discussion around BTW sorry for continually adding more commits, I've kinda been using this a dumping ground 😁. |
|
Yeah, seems good to go. Thank you! |
|
Woohoo, thanks! |
No description provided.