Skip to content

Fix some clippy warnings #466

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

Merged
merged 1 commit into from
Oct 8, 2017
Merged

Fix some clippy warnings #466

merged 1 commit into from
Oct 8, 2017

Conversation

jacwah
Copy link
Contributor

@jacwah jacwah commented Oct 7, 2017

In response to #458.

I did not fix two of clippy's warnings:

warning: returning the result of a let binding from a block. Consider returning the expression directly.
   --> src/renderer/html_handlebars/hbs_renderer.rs:137:9
    |
137 |         rendered
    |         ^^^^^^^^
    |
    = note: #[warn(let_and_return)] on by default
note: this expression can be directly returned
   --> src/renderer/html_handlebars/hbs_renderer.rs:135:24
    |
135 |         let rendered = add_playpen_pre(&rendered, playpen_config);
    |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.165/index.html#let_and_return

I think the code is more readable as it is currently than clippy's suggestion. Should I add
#[cfg_attr(feature = "cargo-clippy", allow(let_and_return))] or are warnings okay here?

warning: large size difference between variants
   --> src/lib.rs:104:5
    |
104 | /     error_chain!{
105 | |         foreign_links {
106 | |             Io(::std::io::Error);
107 | |             HandlebarsRender(::handlebars::RenderError);
...   |
117 | |         }
118 | |     }
    | |_____^
    |
    = note: #[warn(large_enum_variant)] on by default
    = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.165/index.html#large_enum_variant
help: consider boxing the large fields to reduce the total size of the enum
    |
113 | err : Box<$ foreign_link_error_path> ) {
    |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Not sure what to do about this one. Boxing the errors seems more invasive, so I'll leave that decision to someone else. I don't see why any of these variants should be large though.

Copy link
Contributor

@budziq budziq left a comment

Choose a reason for hiding this comment

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

lgtm. Thanks for the PR @jacwah !

@azerupi azerupi merged commit ba7d402 into rust-lang:master Oct 8, 2017
@azerupi
Copy link
Contributor

azerupi commented Oct 8, 2017

Thanks for the fixes! :)

@projektir
Copy link
Contributor

projektir commented Oct 9, 2017

Should I add
#[cfg_attr(feature = "cargo-clippy", allow(let_and_return))] or are warnings okay here?

I think I would. I have been in that part of the code and I agree it's more readable the way it is, but overall the clippy rule is good. We don't want "accepted" warnings cluttering up things, I don't think.

Not sure about the Box one, though.

Generally, having "accepted" warnings is bad in my opinion (then you have to mentally exclude them every time you run clippy again) so finding some way to make them go away is preferred.

@jacwah
Copy link
Contributor Author

jacwah commented Oct 10, 2017

Regarding large_enum_variant: handlebars::TemplateError is the culprit here. Boxing it halves the size of mdbook::errors::Error from 256 to 128 bytes on my x86_64 machine.

Type mem::size_of
io::Error 16
RenderError 96
TemplateError 224
FromUtf8Error 40
mdbook::errors::Error 256

After boxing TemplateError:

Type mem::size_of
mdbook::errors::Error 128

Boxing RenderError too makes only a marginal difference:

Type mem::size_of
mdbook::errors::Error 112

Boxing can be done transparently like this:

--- a/src/lib.rs
+++ b/src/lib.rs
@@ -104,8 +104,8 @@ pub mod errors {
     error_chain!{
         foreign_links {
             Io(::std::io::Error);
             HandlebarsRender(::handlebars::RenderError);
-            HandlebarsTemplate(::handlebars::TemplateError);
+            HandlebarsTemplate(Box<::handlebars::TemplateError>);
             Utf8(::std::string::FromUtf8Error);
         }
 
@@ -116,4 +116,10 @@ pub mod errors {
             }
         }
     }
+
+    impl From<::handlebars::TemplateError> for Error {
+        fn from(e: ::handlebars::TemplateError) -> Error {
+            From::from(Box::new(e))
+        }
+    }
 }

jacwah referenced this pull request in jacwah/mdBook Oct 12, 2017
@jacwah jacwah mentioned this pull request Oct 12, 2017
Ruin0x11 pushed a commit to Ruin0x11/mdBook that referenced this pull request Aug 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants