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

Implement Error for TryReserveError #69792

Merged
merged 4 commits into from
Mar 12, 2020

Conversation

LenaWil2
Copy link
Contributor

@LenaWil2 LenaWil2 commented Mar 7, 2020

I noticed that the Error trait wasn't implemented for TryReserveError. (#48043)

Not sure if the error messages and code style are 100% correct, it's my first time contributing to the Rust std.

@JohnTitor JohnTitor added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 7, 2020
@@ -77,6 +78,22 @@ impl From<LayoutErr> for TryReserveError {
}
Copy link
Contributor Author

@LenaWil2 LenaWil2 Mar 7, 2020

Choose a reason for hiding this comment

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

Maybe it's possible to return the LayoutErr as cause and source of the TryReserveError when passed here, although it would make the struct bigger.

src/libstd/error.rs Outdated Show resolved Hide resolved
@LenaWil2 LenaWil2 requested a review from sfackler March 10, 2020 10:36
Comment on lines +89 to +92
TryReserveError::CapacityOverflow => {
" because the computed capacity exceeded the collection's maximum"
}
TryReserveError::AllocError { .. } => " because the memory allocator returned a error",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do dislike that this isn't formatted consistently, but rustfmt formatted it this way.

Copy link
Member

Choose a reason for hiding this comment

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

The cleaner approach would be to move the match out of the write_str call and just assign the string to a variable IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't help with the formatting though, rustfmt transforms this:

#[unstable(feature = "try_reserve", reason = "new API", issue = "48043")]
impl Display for TryReserveError {
    fn fmt(
        &self,
        fmt: &mut core::fmt::Formatter<'_>,
    ) -> core::result::Result<(), core::fmt::Error> {
        fmt.write_str("memory allocation failed")?;
        let reason = match &self {
            TryReserveError::CapacityOverflow => " because the computed capacity exceeded the collection's maximum",
            TryReserveError::AllocError { .. } => {
                " because the memory allocator returned a error"
            }
        };
        fmt.write_str(reason)
    }
}

Into this:

#[unstable(feature = "try_reserve", reason = "new API", issue = "48043")]
impl Display for TryReserveError {
    fn fmt(
        &self,
        fmt: &mut core::fmt::Formatter<'_>,
    ) -> core::result::Result<(), core::fmt::Error> {
        fmt.write_str("memory allocation failed")?;
        let reason = match &self {
            TryReserveError::CapacityOverflow => {
                " because the computed capacity exceeded the collection's maximum"
            }
            TryReserveError::AllocError { .. } => " because the memory allocator returned a error",
        };
        fmt.write_str(reason)
    }
}

Copy link
Contributor Author

@LenaWil2 LenaWil2 Mar 11, 2020

Choose a reason for hiding this comment

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

I tried to submit code in which both statements used braces, but that makes one of the checks fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's fine now, btw, it's a really minor issue.

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2020-03-11T16:32:46.0800403Z ========================== Starting Command Output ===========================
2020-03-11T16:32:46.0804545Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/b8cb0517-4ea3-42c2-b481-e19e00785563.sh
2020-03-11T16:32:46.0805076Z 
2020-03-11T16:32:46.0808922Z ##[section]Finishing: Disable git automatic line ending conversion
2020-03-11T16:32:46.0827722Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/69792/merge to s
2020-03-11T16:32:46.0831004Z Task         : Get sources
2020-03-11T16:32:46.0831248Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-03-11T16:32:46.0831530Z Version      : 1.0.0
2020-03-11T16:32:46.0831851Z Author       : Microsoft
---
2020-03-11T16:32:47.3241867Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-03-11T16:32:47.3248187Z ##[command]git config gc.auto 0
2020-03-11T16:32:47.3251124Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-03-11T16:32:47.3254999Z ##[command]git config --get-all http.proxy
2020-03-11T16:32:47.3262404Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/69792/merge:refs/remotes/pull/69792/merge

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

@sfackler
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 12, 2020

📌 Commit 2c90a37 has been approved by sfackler

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 12, 2020
bors added a commit that referenced this pull request Mar 12, 2020
Rollup of 10 pull requests

Successful merges:

 - #68899 (Add Display and Error impls for proc_macro::LexError)
 - #69011 (Document unsafe blocks in core::fmt)
 - #69674 (Rename DefKind::Method and TraitItemKind::Method )
 - #69705 (Toolstate: remove redundant beta-week check.)
 - #69722 (Tweak output for invalid negative impl AST errors)
 - #69747 (Rename rustc guide)
 - #69792 (Implement Error for TryReserveError)
 - #69830 (miri: ICE on invalid terminators)
 - #69921 (rustdoc: remove unused import)
 - #69945 (update outdated comment)

Failed merges:

r? @ghost
@bors bors merged commit d21320c into rust-lang:master Mar 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants