Skip to content

Remove faulty shortcut in rewrite_string_lit #241

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

Merged
merged 1 commit into from
Sep 1, 2015
Merged

Remove faulty shortcut in rewrite_string_lit #241

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/comment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ mod test {
check("/* comment */ some text /* more commentary */ result", "result", Some(46));
check("sup // sup", "p", Some(2));
check("sup", "x", None);
check("π? /**/ π is nice!", "π is nice", Some(9));
check(r#"π? /**/ π is nice!"#, r#"π is nice"#, Some(9));
check("/*sup yo? \n sup*/ sup", "p", Some(20));
check("hel/*lohello*/lo", "hello", None);
check("acb", "ab", None);
Expand Down
10 changes: 2 additions & 8 deletions src/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use types::rewrite_path;
use items::{span_lo_for_arg, span_hi_for_arg, rewrite_fn_input};

use syntax::{ast, ptr};
use syntax::codemap::{CodeMap, Pos, Span, BytePos, mk_sp};
use syntax::codemap::{CodeMap, Span, BytePos, mk_sp};
use syntax::visit::Visitor;

impl Rewrite for ast::Expr {
Expand Down Expand Up @@ -831,13 +831,7 @@ fn rewrite_string_lit(context: &RewriteContext,
if context.config.format_strings == false {
return Some(context.snippet(span));
}
// Check if there is anything to fix: we always try to fixup multi-line
// strings, or if the string is too long for the line.
let l_loc = context.codemap.lookup_char_pos(span.lo);
let r_loc = context.codemap.lookup_char_pos(span.hi);
if l_loc.line == r_loc.line && r_loc.col.to_usize() <= context.config.max_width {
return Some(context.snippet(span));
}

let fmt = StringFormat {
Copy link
Member

Choose a reason for hiding this comment

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

Can we consider the position after formatting, rather than not checking at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could, but it would produce the same output as rewriting the string.

Copy link
Member

Choose a reason for hiding this comment

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

iirc, string formatting can be expensive, so skipping it might be a win. OTOH, checking a multi-line string is within bounds is still O(n), so this might be premature optimisation.

opener: "\"",
closer: "\"",
Expand Down
3 changes: 3 additions & 0 deletions tests/source/string-lit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,8 @@ not
remove
formatting"#;

let xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx =
funktion("yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy");

"stuff"
}
5 changes: 5 additions & 0 deletions tests/target/string-lit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,5 +25,10 @@ not
remove
formatting"#;

let xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx = funktion("yyyyyyyyyyyyyyyyyyyyy\
yyyyyyyyyyyyyyyyyyyyy\
yyyyyyyyyyyyyyyyyyyyy\
yyyyyyyyyy");

Copy link
Member

Choose a reason for hiding this comment

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

this doesn't look like an improvement, but I guess we need more infra to fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not an improvement on the test source, but it is an improvement on how rustfmt would previously format this. It would put everything on a single line.

Copy link
Member

Choose a reason for hiding this comment

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

:-)

"stuff"
}