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

std: Add some implementation of common traits #15564

Merged
merged 1 commit into from
Jul 11, 2014

Conversation

alexcrichton
Copy link
Member

  • semver::Version is now Eq, Ord, and Hash
  • Path is now PartialOrd and Ord

@@ -79,7 +79,7 @@ impl fmt::Show for Identifier {


/// Represents a version number conforming to the semantic versioning scheme.
#[deriving(Clone)]
#[deriving(Clone, Hash, Eq)]
Copy link
Member

Choose a reason for hiding this comment

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

Should Hash match PartialEq/Ord? i.e. there are Versions that are equal under either of those two traits for but which have different hashes, due to the build metadata.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch!

@huonw
Copy link
Member

huonw commented Jul 10, 2014

cc #15294

@alexcrichton
Copy link
Member Author

Updated with feedback.

#[allow(missing_doc)]
pub enum Identifier {
Numeric(uint),
AlphaNumeric(String)
}

impl cmp::PartialOrd for Identifier {
#[inline]
fn partial_cmp(&self, other: &Identifier) -> Option<Ordering> {
Copy link
Member

Choose a reason for hiding this comment

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

It seems important to preserve this exact ordering logic here to be consistent with the semver spec. Are we sure we want to depend on the internals of deriving for that?

Copy link
Member Author

Choose a reason for hiding this comment

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

This looks pretty straightforward and I think that we should be able to rely on the order for an enum as "smallest listed first" as other types like Option depend on it as well.

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be eminently sensible for us to define #[deriving] like that (modulo explicit discriminants #15523): if someone wants to get a super-optimised comparison that isn't necessarily the declaration-order, they can implement the trait manually.

- semver::Version is now Eq, Ord, and Hash
- Path is now PartialOrd and Ord
bors added a commit that referenced this pull request Jul 11, 2014
- semver::Version is now Eq, Ord, and Hash
- Path is now PartialOrd and Ord
@bors bors closed this Jul 11, 2014
@bors bors merged commit 8aa8ca7 into rust-lang:master Jul 11, 2014
@alexcrichton alexcrichton deleted the moar-hash branch July 11, 2014 21:46
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 18, 2023
…ubcrates-rustfmt-toml-with-custom-command, r=Veykril

internal: use current folder's `rustfmt.toml` with all rustfmt configurations

## Introduction

Resolves rust-lang/rust-analyzer#15540. I moved the `chdir` functionality outside of the `match` to ensure that this functionality wouldn’t fall through again. As part of this PR, I also changed `from_proto::file_range` to accept a `TextDocumentIdentifier` by reference instead of by value, but I can undo this change if desired.

## Testing

I added a `rustfmt.toml` will the contents below at `crates/rust-analyzer/rustfmt.toml`:


```toml
reorder_modules = false
use_small_heuristics = "Max"
# this is the only difference from the `rustfmt.toml` at the root of the repo
tab_spaces = 8
```

In addition, I've also added `"rust-analyzer.rustfmt.overrideCommand": ["rustfmt"]` to my VS Code configuration.

With the above changes, saving `crates/rust-analyzer/src/handlers/request.rs` results in 8-space indentation. Meanwhile, saving `crates/toolchain/src/lib.rs` _does not_ result in any formatting changes.
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.

4 participants