Skip to content

Conversation

@topecongiro
Copy link
Contributor

Currently rustfmt ignores file lines check for items.
Closes #1835.

Copy link
Member

@nrc nrc left a comment

Choose a reason for hiding this comment

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

A couple of minor code-tidiness comments inline

src/visitor.rs Outdated

impl<'a> FmtVisitor<'a> {
// Return true if the given span does not intersect with file lines.
fn skip_span(&self, span: Span) -> bool {
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 use the context version of skip_span rather than duplicating the code?

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, though we need to call get_context() everytime, which is a bit unfortunate.

src/visitor.rs Outdated
pub fn visit_item(&mut self, item: &ast::Item) {
if self.skip_span(item.span) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

We could make this a macro and wouldn't have to repeat the if ... return.

@topecongiro
Copy link
Contributor Author

Rebased with the change to use macro.

@emilio
Copy link
Contributor

emilio commented Jul 29, 2017

I was super-excited to try this with bindgen, but this causes a bunch of undesired effects.

Current STR (will try to find something more reduced later today):

diff --git a/src/lib.rs b/src/lib.rs
index 3a85382..1b68a0a 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -1268,7 +1268,7 @@ fn filter_builtins(ctx: &BindgenContext, cursor: &clang::Cursor) -> bool {
 
 /// Parse one `Item` from the Clang cursor.
 pub fn parse_one(ctx: &mut BindgenContext,
-                 cursor: clang::Cursor,
+             cursor: clang::Cursor,
                  parent: Option<ItemId>)
                  -> clang_sys::CXChildVisitResult {
     if !filter_builtins(ctx, &cursor) {
rustfmt --file-lines '[{"range": [1268, 1274], "file": "src/lib.rs"}]' src/lib.rs

Expected: Only the parse_one signature is modified.

Actual: A lot of use statements and whitespace is missing, and also lines with comments misindented.

@emilio
Copy link
Contributor

emilio commented Jul 29, 2017

This helps not to get them skipped, but the use statements still get reformatted / reordered:

diff --git a/src/utils.rs b/src/utils.rs
index 9c1717e..47ce5e7 100644
--- a/src/utils.rs
+++ b/src/utils.rs
@@ -343,6 +343,7 @@ macro_rules! skip_out_of_file_lines_range {
 macro_rules! skip_out_of_file_lines_range_visitor {
     ($self:ident, $span:expr) => {
         if out_of_file_lines_range!($self, $span) {
+            $self.push_rewrite($span, None);
             return;
         }
     }

Disclaimer, no clue about whether it's the right fix :-)

@emilio
Copy link
Contributor

emilio commented Jul 29, 2017

Ok, I also got a patch for the other issue, I'll try to add some tests :-)

@emilio
Copy link
Contributor

emilio commented Jul 29, 2017

I opened #1841 with my fixes on top of this.

Also, thanks @topecongiro! I was about to report too much stuff being formatted with file-lines, and you already had a fix up for review, that's awesome! :)

@nrc nrc merged commit 7c9db55 into rust-lang:master Jul 30, 2017
@nrc
Copy link
Member

nrc commented Jul 30, 2017

Thanks for the changes!

@topecongiro topecongiro deleted the issue-1835 branch May 23, 2018 15:24
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.

3 participants