AaronKutch commented 5 years ago

I compared the .rustfmt.toml I made with yours and I am suggesting a few changes. I placed the keys in alphabetical order. I think using imports_layout = "HorizontalVertical" is better since it uses horizontal layout for 2-4 imports (typically) and a vertical layout for longer than that. I think keeping comment width at 80 is a good idea, however I would prefer the default max_width = 100. I also used a few nightly keys to improve comment formatting.

coveralls commented 5 years ago

Coverage decreased (-0.1%) to 75.83% when pulling 494faba35265cf465ac7bbe6541737f9e3faab7a on AaronKutch:new_rustfmt into 10aa554e23fe2ecb97490b0074200b895276753c on Robbepop:master.

codecov-io commented 5 years ago

Codecov Report

Merging #51 into master will decrease coverage by 0.34%. The diff coverage is 34.82%.

@@            Coverage Diff             @@
##           master      #51      +/-   ##
==========================================
- Coverage   76.12%   75.77%   -0.35%     
==========================================
  Files          22       21       -1     
  Lines        5340     5288      -52     
==========================================
- Hits         4065     4007      -58     
- Misses       1275     1281       +6

Impacted Files	Coverage Δ
src/uint.rs	`0% <ø> (ø)`	:arrow_up:
src/bitwidth.rs	`89.06% <ø> (-9.51%)`	:arrow_down:
src/storage.rs	`100% <ø> (ø)`	:arrow_up:
src/digit_seq.rs	`77.77% <ø> (ø)`	:arrow_up:
src/apint/casting.rs	`77.72% <ø> (+1.33%)`	:arrow_up:
src/apint/serde_impl.rs	`78.94% <ø> (ø)`	:arrow_up:
src/bitpos.rs	`100% <ø> (ø)`	:arrow_up:
src/apint/shift.rs	`100% <ø> (ø)`	:arrow_up:
src/int.rs	`0% <ø> (ø)`	:arrow_up:
src/radix.rs	`88.88% <ø> (ø)`	:arrow_up:
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 10aa554...494faba. Read the comment docs.

Robbepop commented 5 years ago

I compared the .rustfmt.toml I made with yours and I am suggesting a few changes. I placed the keys in alphabetical order. I think using imports_layout = "HorizontalVertical" is better since it uses horizontal layout for 2-4 imports (typically) and a vertical layout for longer than that. I think keeping comment width at 80 is a good idea, however I would prefer the default max_width = 100. I also used a few nightly keys to improve comment formatting.

Can you please precisely list what changes you made exactly? I was even thinking about setting max_width to 80 but that would have been too disruptive for now. In my opinion long lines are just another indicator of code smell. Also I do not want an ultra-flat screen for code review or for looking at diffs.

I also find it easier to look at horizontal lists to grab an overview that's why I chose imports_layout = "Vertical" but I might go with you. Let me think about it.

AaronKutch commented 5 years ago

I decided to go with your max_width = 90. The differences between my .rustfmt.toml and the current one is:

imports_layout = "HorizontalVertical"
version = "Two" ( changes barely anything)
error_on_line_overflow = true (I manually formatted in some places to make this work)
error_on_unformatted = true
report_fixme = "Never" (I always look at the todo's manually)
report_todo = "Never"
newline_style = "Unix" # changed, prevents problems when a file is created on Windows I also added a few new keys such as:
struct_lit_single_line = true
format_macro_bodies = true
format_code_in_doc_comments = true

Robbepop commented 5 years ago

My comments on your proposals below.

* imports_layout = "HorizontalVertical"

I will think about this. I might prefer this over Vertical if we can manage to reduce the line noise to line widths of at most 40 columns or so. I really hate to read too many comma-separated names in a single line.

* error_on_line_overflow = true (I manually formatted in some places to make this work)
* error_on_unformatted = true

Maybe we want this for CI instead since I do not want to pollute too many warnings into errors as I have made the experience of this causing troubles with prototyping new code.

* report_fixme = "Never" (I always look at the todo's manually)
* report_todo = "Never"

My experience in the past with these flags were very positive. But also maybe we want to provide those as errors in CI. I'll think about it.

* newline_style = "Unix" # changed, prevents problems when a file is created on Windows

If this really fixes problem I wonder why it isn't the default. Can you elaborate or link to an issue?

* struct_lit_single_line = true

I like this but it is unstable (as some other already used flags) and since apint is also for the stable channel we have to be careful what configurations of rustfmt we actually are able to use.

* format_macro_bodies = true
* format_code_in_doc_comments = true

I have had bad experience with these in the past and won't accept them therefore.

AaronKutch commented 5 years ago

I am using newline_style = "Unix" to enforce LF line endings throughout the codebase. For some reason (probably due to git or one of my editors trying to change LF back to CRLF), I always have a problem with the files being a mix of CRLF and LF. I just checked manually, and some files are still CRLF for some reason. I don't know if it is git's problem or some bug with rustfmt. I just force pushed a commit to fix it.

Maybe we want this for CI instead since I do not want to pollute too many warnings into errors as I have made the experience of this causing troubles with prototyping new code.

I only found about five places in the entire codebase that rustfmt was unable to format, mainly due to comments in the middle of method chains.

My experience in the past with these flags were very positive. But also maybe we want to provide those as errors in CI. I'll think about it.

These should be put in CI.

I like this but it is unstable (as some other already used flags) and since apint is also for the stable channel we have to be careful what configurations of rustfmt we actually are able to use.

rustfmt is purely a developer side thing, so I don't think there is any issues with the unstable flags.

I have had bad experience with these in the past and won't accept them therefore.

I have experienced only positive things, except for the documentation for from_str_radix in serialization.rs, where I had to change the comments from being inline to being in front of what they are commenting on

AaronKutch commented 5 years ago

Github has a feature to view all non-whitespace changes

AaronKutch commented 5 years ago

I fixed the merge conflicts. The Fix rustfmt errors contains all the manual reformatting.

AaronKutch commented 5 years ago

There are warnings after the rebase that appear to be from your commits. I will fix them sometime next week.

AaronKutch commented 4 years ago

Are you still not satisfied with my changes?

Robbepop commented 4 years ago

Are you still not satisfied with my changes?

Have to give it another look. Didn't look into this for a while. But what I can quickly say is that I definitely find

use crate::{
    apint::ApInt,
    bitwidth::BitWidth,
    digit::Digit,
};

... more readable than ...

use crate::{apint::ApInt, bitwidth::BitWidth, digit::Digit};

AaronKutch commented 4 years ago

Should I squash the last three commits?

AaronKutch commented 4 years ago

wait, I just realized I need to rebase on top of the into_iter change, let me fix it

AaronKutch commented 4 years ago

That should be it, except I am getting the warning

warning: unused import: `libm::F64Ext`
   --> src\apint\serialization.rs:277:13
    |
277 |         use libm::F64Ext as _

I presume this is from the commit that supports no_std

Robbepop commented 4 years ago

That should be it, except I am getting the warning
warning: unused import: `libm::F64Ext`
   --> src\apint\serialization.rs:277:13
    |
277 |         use libm::F64Ext as _
I presume this is from the commit that supports no_std

Then maybe we simply need a cfg no_std for the import.

AaronKutch commented 4 years ago

I fixed the warning for most cases and a compilation error for serde_support with no_std. For some bizarre reason, when I run with cargo check --features=serde_support,libm_0 --no-default-features, it gives an unused warning as if the F64Ext isn't needed any more. I cannot see anywhere where adding serde_support pulls in a way for the log2 function to not need the F64Ext in no_std. It doesn't do the same thing with rand_support.

AaronKutch commented 4 years ago

I am almost done with getting my division algorithms into compiler-builtins. When are you going to merge this?

Robbepop commented 4 years ago

I am almost done with getting my division algorithms into compiler-builtins. When are you going to merge this?

Sorry, forgot about this PR. Can you send me a link to your compiler-builtin PR? I will review this again and eventually merge this today or tomorrow - I will keep the TAB open so that I don't forget again. ;)

AaronKutch commented 4 years ago

https://github.com/rust-lang/compiler-builtins/pull/332 This will greatly improve u128 division and divisions for 32 bit platforms

AaronKutch commented 4 years ago

All I want for Christmas is a rapid PR review cycle

Robbepop commented 4 years ago

All I want for Christmas is a rapid PR review cycle

You can help this rapid PR review cycle by chunking your commits in a way that makes them easy to review. For example in this PR the single most important file is obviously .rustfmt.toml. However, you made two changes to it: Changing some options and changing ordering: The first change is severe, the other change is trivial to review. However, since you merged both changes into a single commit it made it hard to extract the severe parts out of the many trivial ones. So by simply phasing out those 2 changes into two separate commits you wouldn't force a reviewer to go through every single line, check what changed and if so check the Rust docs but would simply pre-filter the important parts out.

AaronKutch commented 4 years ago

However, since you merged both changes into a single commit it made it hard to extract the severe parts out of the many trivial ones.

What happened here was I completely replaced the existing rustfmt.toml with my original rustfmt.toml I had made for the reorganization PR that never made it. I should have put yours in alphabetical order in one commit, and then replaced it with another commit.

AaronKutch commented 4 years ago

I had a really bad sickness recently and it sapped all my energy and is distorting my judgement. I will redo the commits a third time to fix your problems

AaronKutch commented 4 years ago

I split up the formatting itself to show you what the

format_code_in_doc_comments = true # changed
format_macro_bodies = true # changed
format_macro_matchers = true # changed
format_strings = true # changed

part does so you can see it is useful

AaronKutch commented 4 years ago

cargo fmt returns nothing except for TODO warnings after the last commit.

Robbepop commented 4 years ago

I had a really bad sickness recently and it sapped all my energy and is distorting my judgement. I will redo the commits a third time to fix your problems

It is fine. I have already went through the whole changes. I will give it another look tomorrow (very tired rn) and probably approve and merge then. Thank you again for all your work on this crate!

Robbepop / apint

Better rustfmt #51

Codecov Report