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.

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

A Case Against Separators #3798

Closed
mitsuhiko opened this issue Oct 18, 2012 · 5 comments
Closed

A Case Against Separators #3798

mitsuhiko opened this issue Oct 18, 2012 · 5 comments

Comments

@mitsuhiko
Copy link
Contributor

There are two places where rust currently users separators instead of terminators where the constructs are usually line terminated: struct fields and match arms:

struct Foo {
   left: int,
   right: int
}

let x = match foo() {
  None => 0,
  Some(num) => num
}

Here is why I think this is a bad idea: diffs. At the moment if you add a field to the struct at the end most diff tools will show two modified lines which causes conflicts if two developers add an entry. This is pretty annoying. For match there is currently a way to avoid that by wrapping the arms in {} in which case there is no terminator at all and the diff succeeds. The struct however at the moment requires the commas. I am not sure why, but I think it's because of the initializer syntax.

I understand that an argument against that could be to always add a terminating comma but history has shown that developers will not do that because they don't see the benefit of it (and it also looks unexpected).

@mitsuhiko
Copy link
Contributor Author

Example diff showing the problem:

$ diff -u a.rs b.rs
--- a.rs 2012-10-18 02:31:09.000000000 +0100
+++ b.rs 2012-10-18 02:31:18.000000000 +0100
@@ -1,3 +1,4 @@
 struct Foo {
- field_1: int
+ field_1: int,
+ field_2: int
 }

@Blei
Copy link
Contributor

Blei commented Oct 18, 2012

Note that you can terminate each field/match clause with a ,. I.e., the following is legal:

struct Foo {
   a: int,
   b: int,
}

fn bar(c: uint) {
    match c {
        0 => io::println("0"),
        1 => io::println("1"),
        2 => io::println("2"),
        _ => io::println("many"),
    }
}

@mitsuhiko
Copy link
Contributor Author

That is mentioned in the comment. My point is: programmers optimize towards laziness.

@bblum
Copy link
Contributor

bblum commented Oct 19, 2012

I think the optional comma at the end is useful precisely for this reason. Programmers who care about diffs can use them; if you're running a project and you think it's important, you can make it part of the house style guide.

It's important for it to be optional, too, though, especially for concise one-liners. foo { f: 1, g: 2, } and match bar() { Some(x) => x, None => fail, } just look bad.

@catamorphism
Copy link
Contributor

Insufficient interest, closing.

bors pushed a commit to rust-lang-ci/rust that referenced this issue May 15, 2021
RalfJung pushed a commit to RalfJung/rust that referenced this issue Aug 18, 2024
…, r=saethlin

miri-script: use --remap-path-prefix to print errors relative to the right root

Inspired by rust-lang/rust-clippy#13232, this makes it so that when cargo-miri fails to build, `./miri check` will print errors with paths like `cargo-miri/src/setup.rs`. That means we can get rid of the miri-symlink-hacks and instead tell RA to just always invoke the `./miri clippy` script just once, in the root.

This means that we can no longer share a target dir between cargo-miri and miri as the RUSTFLAGS are different to crates that are shared in the dependency tree need to be built twice with two different flags. `miri-script` hence now has to set the MIRI environment variable to tell the `cargo miri setup` invocation where to find Miri.

I also made it so that errors in miri-script itself are properly shown in RA, for which the `./miri` shell wrapper needs to set the right flags.
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

No branches or pull requests

4 participants