Skip to content
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.

Sign up for GitHub

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

Jump to bottom

rustdoc: add line breaks to where clauses a la rustfmt #37190

Merged
merged 6 commits into from
Nov 12, 2016
Merged

rustdoc: add line breaks to where clauses a la rustfmt #37190

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4a6921e
rustdoc: add line breaks to where clauses a la rustfmt
Oct 13, 2016
c6ab685
fix spurious </span> appearing before the opening tag
Oct 13, 2016
42f28d3
rustdoc: apply proper indent when where clauses aren't on their own line
Oct 15, 2016
07b27bb
rustdoc: break where clauses onto their own line if they don't have e…
Oct 15, 2016
43abad4
rustdoc: if a where clause gets any lines, it gets its own line
Oct 16, 2016
61cc870
rustdoc: make Method/WhereClause wrappers use usize for indents
Oct 17, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 53 additions & 18 deletions src/librustdoc/html/format.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ pub struct MutableSpace(pub clean::Mutability);
#[derive(Copy, Clone)]
pub struct RawMutableSpace(pub clean::Mutability);
/// Wrapper struct for emitting a where clause from Generics.
pub struct WhereClause<'a>(pub &'a clean::Generics);
pub struct WhereClause<'a>(pub &'a clean::Generics, pub String);
Copy link
Member

Choose a reason for hiding this comment

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

I'd be in favor of making this:

pub struct WhereClause<'a> {
    pub generics: &'a clean::Generics,
    pub pad: String,
};

So we're explicit about what the fields are instead of a collection of unnamed data structures.

Copy link
Member Author

Choose a reason for hiding this comment

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

No big deal to me either way. Looking at it again now, I kinda want to change that String (and Method's str, above) to integers since all they're conveying is the number of spaces to prepend onto new lines. They're passed differently, too, but handled the same.

/// Wrapper struct for emitting type parameter bounds.
pub struct TyParamBounds<'a>(pub &'a [clean::TyParamBound]);
/// Wrapper struct for emitting a comma-separated list of items
Expand Down Expand Up @@ -157,52 +157,69 @@ impl fmt::Display for clean::Generics {

impl<'a> fmt::Display for WhereClause<'a> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
let &WhereClause(gens) = self;
let &WhereClause(gens, ref pad) = self;
if gens.where_predicates.is_empty() {
return Ok(());
}
let mut clause = String::new();
if f.alternate() {
f.write_str(" ")?;
clause.push_str(" where ");
} else {
f.write_str(" <span class='where'>where ")?;
clause.push_str(" <span class='where'>where ");
}
for (i, pred) in gens.where_predicates.iter().enumerate() {
if i > 0 {
f.write_str(", ")?;
if f.alternate() {
clause.push_str(", ");
} else {
clause.push_str(",<br>");
}
Copy link
Member

Choose a reason for hiding this comment

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

Subjective, but these sort of patterns can be written:

clause.push_str(if f.alternate() {
    ", "
} else {
    ",<br>"
})

Copy link
Member Author

Choose a reason for hiding this comment

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

Personal preference, I guess. It it part of a broader style guideline somewhere? At that point it's a ternary expression, too, and can be collapsed onto one line.

Copy link
Member

Choose a reason for hiding this comment

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

It it part of a broader style guideline somewhere?

I don't think so; what you have is fine.

}
match pred {
&clean::WherePredicate::BoundPredicate { ref ty, ref bounds } => {
let bounds = bounds;
if f.alternate() {
write!(f, "{:#}: {:#}", ty, TyParamBounds(bounds))?;
clause.push_str(&format!("{:#}: {:#}", ty, TyParamBounds(bounds)));
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to use write! instead of .push_str(&format!( to avoid an extra string allocation. For example, by importing std::fmt::Write this can be write!(clause, "{:#}: {:#}", ty, TyParamBounds(bounds)).unwrap();.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about that adjustment as I was going through - I even asked in IRC whether it was possible to use write!() on a String when I was doing it. I decided against writing it in right then because there were other places (not just in my additions here or the previous PR) that used this pattern, so my sense of purism wanted to separate that change to its own logical unit, after this one got dealt with.

} else {
write!(f, "{}: {}", ty, TyParamBounds(bounds))?;
clause.push_str(&format!("{}: {}", ty, TyParamBounds(bounds)));
}
}
&clean::WherePredicate::RegionPredicate { ref lifetime,
ref bounds } => {
write!(f, "{}: ", lifetime)?;
clause.push_str(&format!("{}: ", lifetime));
for (i, lifetime) in bounds.iter().enumerate() {
if i > 0 {
f.write_str(" + ")?;
clause.push_str(" + ");
}

write!(f, "{}", lifetime)?;
clause.push_str(&format!("{}", lifetime));
}
}
&clean::WherePredicate::EqPredicate { ref lhs, ref rhs } => {
if f.alternate() {
write!(f, "{:#} == {:#}", lhs, rhs)?;
clause.push_str(&format!("{:#} == {:#}", lhs, rhs));
} else {
write!(f, "{} == {}", lhs, rhs)?;
clause.push_str(&format!("{} == {}", lhs, rhs));
}
}
}
}
if !f.alternate() {
f.write_str("</span>")?;
clause.push_str("</span>");
let plain = format!("{:#}", self);
if plain.len() + pad.len() > 80 {
let padding = if pad.len() + 25 > 80 {
clause = clause.replace("class='where'", "class='where fmt-newline'");
repeat("&nbsp;").take(8).collect::<String>()
} else {
repeat("&nbsp;").take(pad.len() + 6).collect::<String>()
};
clause = clause.replace("<br>", &format!("<br>{}", padding));
} else {
clause = clause.replace("<br>", " ");
}
}
Ok(())
write!(f, "{}", clause)
}
}

Expand Down Expand Up @@ -718,30 +735,44 @@ impl fmt::Display for clean::Type {
}

fn fmt_impl(i: &clean::Impl, f: &mut fmt::Formatter, link_trait: bool) -> fmt::Result {
let mut plain = String::new();

if f.alternate() {
write!(f, "impl{:#} ", i.generics)?;
} else {
write!(f, "impl{} ", i.generics)?;
}
plain.push_str(&format!("impl{:#} ", i.generics));
Copy link
Member

Choose a reason for hiding this comment

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

Could this just be:

let mut plain = format!("impl{:#} ", i.generics);

...and then getting rid of the String::new() above?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see why not. I guess my intent was to have the declaration separate since I was going to be pushing more things onto it later.


if let Some(ref ty) = i.trait_ {
write!(f, "{}",
if i.polarity == Some(clean::ImplPolarity::Negative) { "!" } else { "" })?;
if i.polarity == Some(clean::ImplPolarity::Negative) {
write!(f, "!")?;
plain.push_str("!");
}

if link_trait {
fmt::Display::fmt(ty, f)?;
plain.push_str(&format!("{:#}", ty));
} else {
match *ty {
clean::ResolvedPath{ typarams: None, ref path, is_generic: false, .. } => {
let last = path.segments.last().unwrap();
fmt::Display::fmt(&last.name, f)?;
fmt::Display::fmt(&last.params, f)?;
plain.push_str(&format!("{:#}{:#}", last.name, last.params));
}
_ => unreachable!(),
}
}
write!(f, " for ")?;
plain.push_str(" for ");
}

fmt::Display::fmt(&i.for_, f)?;
fmt::Display::fmt(&WhereClause(&i.generics), f)?;
plain.push_str(&format!("{:#}", i.for_));

let pad = repeat(" ").take(plain.len() + 1).collect::<String>();
fmt::Display::fmt(&WhereClause(&i.generics, pad), f)?;
Ok(())
}

Expand Down Expand Up @@ -887,7 +918,11 @@ impl<'a> fmt::Display for Method<'a> {
} else {
output = output.replace("<br>", "");
}
write!(f, "{}", output)
if f.alternate() {
write!(f, "{}", output.replace("<br>", "\n"))
} else {
write!(f, "{}", output)
}
}
}

Expand Down
Loading