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

rustfmt on metadata folder #37349

Merged

bors merged 1 commit into rust-lang:master from srinivasreddy:meta_1

Oct 25, 2016

Contributor

srinivasreddy commented Oct 22, 2016

No description provided.

rust-highfive assigned nikomatsakis

Collaborator

rust-highfive commented Oct 22, 2016

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

Contributor Author

srinivasreddy commented Oct 22, 2016

@nikomatsakis LGTM. What do you think?

Member

eddyb commented Oct 22, 2016

Didn't you already do this in #37350?

Contributor Author

srinivasreddy commented Oct 22, 2016

I split the folder into two PRs since it is huge

Member

eddyb commented Oct 22, 2016

@srinivasreddy There's no size limit to GitHub PRs and it's not helpful to have related changes split.

Also, you'll likely have to wait for #37301 to get merged, in order to not have conflicts.

Contributor Author

srinivasreddy commented Oct 22, 2016

@eddyb I know that. I just wanted to avoid merge conflicts with merged PRs in between. 😄

jseyfried reviewed

View reviewed changes

src/librustc_metadata/creader.rs

    
                  pub fn find_plugin_registrar(&mut self, span: Span, name: &str)

                  pub fn find_plugin_registrar(&mut self,

                                               span: Span,

                                               name: &str)

Contributor

jseyfried Oct 22, 2016

Is there a reason rustfmt gives each argument its own line here?
I think it just wastes vertical space without helping readability.

src/librustc_metadata/creader.rs

    
                                                                 ident: name.to_string(),

                                                                 id: ast::DUMMY_NODE_ID,

                                                                 should_link: false,

                                                             });

Contributor

jseyfried Oct 22, 2016

IMO, the massive rightward drift here is ugly -- I think the original formatting looks much nicer.

src/librustc_metadata/creader.rs

    
                                                                      None,

                                                                      item.span,

                                                                      PathKind::Crate,

                                                                      true);

Contributor

jseyfried Oct 22, 2016

I prefer the original formatting here.

src/librustc_metadata/cstore.rs

    
            @@ -215,20 +217,25 @@ impl CStore {
          
                  // In order to get this left-to-right dependency ordering, we perform a

                  // topological sort of all crates putting the leaves at the right-most

                  // positions.

                  pub fn do_get_used_crates(&self, prefer: LinkagePreference)

                  pub fn do_get_used_crates(&self,

                                            prefer: LinkagePreference)

Contributor

jseyfried Oct 22, 2016

(also here -- no need for the added line break)

src/librustc_metadata/decoder.rs

    
                               id, self.from_id_range, self.to_id_range);

                               id,

                               self.from_id_range,

                               self.to_id_range);

Contributor

jseyfried Oct 22, 2016

IMO, this is more readable as it was with the arguments one one line.

src/librustc_metadata/decoder.rs

    
                              debug!("evaluating the type of {:?}::{:?}", variant.name, field.name);

                              debug!("evaluating the type of {:?}::{:?}",

                                     variant.name,

                                     field.name);

Contributor

jseyfried Oct 22, 2016

Not sure why rustfmt breaks up this line...

src/librustc_metadata/decoder.rs

    
                                     variant.name, field.name, ty);

                                     variant.name,

                                     field.name,

                                     ty);

Contributor

jseyfried Oct 22, 2016

(same here -- I think this is more readable as it was with these arguments on one line)

src/librustc_metadata/decoder.rs

    
                                              });

                                          }

                                      }

                                      continue;

                                  }

                                  EntryKind::Impl(_) | EntryKind::DefaultImpl(_) => continue,

                                  EntryKind::Impl(_) |

                                  EntryKind::DefaultImpl(_) => continue,

Contributor

jseyfried Oct 22, 2016

Needless line break.

src/librustc_metadata/decoder.rs

    
                                              callback(def::Export {

                                                  def: ctor_def,

                                                  name: name,

                                              });

Contributor

jseyfried Oct 22, 2016

I think this is more readable as it was (I think of the struct's fields as named arguments here).

src/librustc_metadata/decoder.rs

    
                                                                end_pos,

                                                                lines,

                                                                multibyte_chars,

                                                                .. } = filemap_to_import;

Contributor

jseyfried Oct 22, 2016

IMO this looked much nicer before with block indenting and a newline after the {.

Contributor

bors commented Oct 23, 2016

☔ The latest upstream changes (presumably #37301) made this pull request unmergeable. Please resolve the merge conflicts.


          Run rustfmt on metadata folder - (1/2)

cac3e5a

Contributor

nikomatsakis commented Oct 24, 2016

@jseyfried I suggest you open some issues on rustfmt =)

Contributor

nikomatsakis commented Oct 24, 2016

@bors r+ rollup

Contributor

bors commented Oct 24, 2016

📌 Commit cac3e5a has been approved by nikomatsakis

sophiajt mentioned this pull request

Rollup of 7 pull requests #37382

Merged

sophiajt pushed a commit to sophiajt/rust that referenced this pull request


          Rollup merge of rust-lang#37349 - srinivasreddy:meta_1, r=nikomatsakis

59b7ea4

rustfmt on metadata folder

bors added a commit that referenced this pull request


          Auto merge of #37382 - jonathandturner:rollup, r=jonathandturner

7a20864

Rollup of 7 pull requests

- Successful merges: #37228, #37304, #37324, #37328, #37336, #37349, #37372
- Failed merges:

bors merged commit cac3e5a into rust-lang:master

srinivasreddy deleted the meta_1 branch

October 25, 2016 03:12

varkor mentioned this pull request

Clarify internal programming style #49567

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet