Skip to content

Automatically enable -Zorbit if -Zincremental is specified. #35159

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

Merged
merged 1 commit into from
Aug 3, 2016

Conversation

michaelwoerister
Copy link
Member

DebuggingOptions {
orbit: true,
.. debugging_opts
}
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit too much, could mutation be used instead?

Copy link
Member

Choose a reason for hiding this comment

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

Easily optimised out.

Copy link
Member

Choose a reason for hiding this comment

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

Though, I agree it would look better with mutation here.

Copy link
Member

Choose a reason for hiding this comment

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

I was talking about code readability, not performance 😆.

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 is a bit too much, could mutation be used instead?

You think? I actually thought that it was more readable that way. The alternative would be to make debugging_opts mutable, but it's within a huge function.

Or would you prefer this:

    let debugging_opts = if debugging_opts.incremental.is_some() &&
                            !debugging_opts.orbit {
        early_warn(error_format, "Automatically enabling `-Z orbit` because \
                                  `-Z incremental` was specified");
        let mut debugging_opts = debugging_opts;
        debugging_opts.orbit = true;
        debugging_opts
    }

I really don't care that much one way or the other, just tell me which version you want :)

Copy link
Member

Choose a reason for hiding this comment

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

I would've expected making debugging_opts mutable to be fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I'll change it.

@nikomatsakis
Copy link
Contributor

r=me once updated

@michaelwoerister michaelwoerister force-pushed the incr-comp-implies-orbit branch from c62a3ed to f900b18 Compare August 1, 2016 15:51
@michaelwoerister
Copy link
Member Author

@bors r=nikomatsakis

@bors
Copy link
Collaborator

bors commented Aug 1, 2016

📌 Commit f900b18 has been approved by nikomatsakis

@bors
Copy link
Collaborator

bors commented Aug 2, 2016

⌛ Testing commit f900b18 with merge 359fdda...

@bors
Copy link
Collaborator

bors commented Aug 2, 2016

💔 Test failed - auto-win-msvc-64-opt

// except according to those terms.

// ignore-pretty
// compile-flags:-Zincremental=tmp/cfail-tests/enable-orbit-for-incr-comp
Copy link
Member

Choose a reason for hiding this comment

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

You need -Zorbit=off now to test this.

@michaelwoerister michaelwoerister force-pushed the incr-comp-implies-orbit branch from f900b18 to 44dbc49 Compare August 2, 2016 21:02
@michaelwoerister
Copy link
Member Author

@bors r=nikomatsakis

Set -Zorbit=off in the test case...

@bors
Copy link
Collaborator

bors commented Aug 2, 2016

📌 Commit 44dbc49 has been approved by nikomatsakis

@bors
Copy link
Collaborator

bors commented Aug 3, 2016

⌛ Testing commit 44dbc49 with merge f495483...

bors added a commit that referenced this pull request Aug 3, 2016
…komatsakis

Automatically enable -Zorbit if -Zincremental is specified.

Fixes #34973

r? @nikomatsakis
@bors bors merged commit 44dbc49 into rust-lang:master Aug 3, 2016
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.

5 participants