-
Notifications
You must be signed in to change notification settings - Fork 232
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 "no-backline" feature #603
Conversation
@@ -108,13 +113,13 @@ where | |||
self.write_newline()?; | |||
} | |||
HardBreak => { | |||
self.write("<br />\n")?; | |||
self.write(&(String::from("<br />") + BACKLINE))?; |
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.
Are these allocating? If so, that seems like a performance hit.
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 const string cancatenation so it should have no impact at runtime (to be confirmed though!).
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.
Ah maybe not. I think String::from
isn't const.
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.
Originally I wanted to use if cfg!(feature = "no-backline")
but found it too noisy. Well in any case, it's another possibility. But the question here is really if you're interested by this feature or not. If you are, I need to add tests too. :)
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 don't think LLVM is smart enough to get rid of the allocation.
Instead, you could make it with concat!
? Like this?
#[cfg(feature = "no-backline")]
macro_rules! opt_backline { ($lit:lit) => { concat!($lit) } }
#[cfg(not(feature = "no-backline"))]
macro_rules! opt_backline { ($lit:lit) => { concat!($lit, "\n") } }
Then this line becomes:
self.write(&(String::from("<br />") + BACKLINE))?; | |
self.write(opt_backline!("<br />"))?; |
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.
Possibly. But considering the small impact, I think it's not worth it currently. Maybe for later on.
I am not sure whether an output size savings of 0.05% to 0.3% is worth the extra complexity this would introduce, especially when we would have to use a bunch of |
Considering the code it adds compared to the result, I don't think it's worth it currently so closing it. |
This is part of an effort to reduce the number of unneeded characters in the generated HTML. So using this feature when generating the
std
gave this result:So surprisingly, the number of characters seems to be quite high on a given file, but the impact on the documentation seems quite small.
So what do you think of this feature? Would you be interested into it? It diverges from the commonmark spec, hence why I suggest to add it as a feature and not a parameter.