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

Show the sign for signed ops on `exact_div` #66148

Merged
merged 1 commit into from Dec 3, 2019

Conversation

@oli-obk
Copy link
Contributor

oli-obk commented Nov 6, 2019

r? @RalfJung Cc https://github.com/rust-lang/miri/pull/961/files#r341842128

I'm fairly unhappy with the duplication and the general effort required for this.

Maybe it would be better to add a display impl for ImmTy?

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Nov 6, 2019

⚠️ Warning ⚠️

  • These commits modify submodules.
@Centril

This comment has been minimized.

Copy link
Member

Centril commented Nov 6, 2019

Maybe it would be better to add a display impl for ImmTy?

No please. :) I think Display impls in the compiler contribute towards implicit dependencies and are harmful wrt. #65031 (comment) due to the orphan rules.

Surely we can de-dup some other way without adding those impls... ^^

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Nov 6, 2019

I think Display impls in the compiler contribute towards implicit dependencies and are harmful wrt. #65031 (comment) due to the orphan rules

I have no idea what this means... Display is defined in libstd so it's a different crate anyway. Also Immediate is not in librustc so it should have a hard time contributing to its compile times.

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Nov 6, 2019

I have no idea what this means... Display is defined in libstd so it's a different crate anyway.

When I was refactoring libsyntax, there were a bunch of implicit dependencies on e.g. impl Display for syntax::ast::Path which in turn used syntax::print::pprust. The problems is if you move pprust out of libsyntax.

Also Immediate is not in librustc so it should have a hard time contributing to its compile times.

Ah OK, great.

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Nov 6, 2019

@oli-obk Scalar has both a Display and a Debug impl already. I think the Display impl is used when printing the invalid value that we encountered during validity checking; the Debug impl is extremely helpful when debugging CTFE stuff as it makes the Miri trace just so much more readable.^^

For Debug I chose to display hex to work around the sign issue, and anyway we don't have enough type information to determine if we should print this as signed or unsigned. But likely the Display impl is just a bad idea and should be removed (it has the same issue as exact_div, not honoring the sign). I think having a Display impl for ImmTy instead and using that both in exact_div and for validity would be a good idea. Or, if it has to be, a to_string method returning String; I don't think we mind some extra allocations when generating the error message (but it seems silly to me to not use omnipresent abstractions like Display).

Also see #65859, that code also pretty-prints MIR values, so probably all of these cases should share code.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 9, 2019

☔️ The latest upstream changes (presumably #66243) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon

This comment has been minimized.

Copy link
Member

JohnCSimon commented Nov 16, 2019

Ping from triage - this PR has sat idle for a while and is now unmergable.
@oli-obk @Centril @RalfJung

@oli-obk oli-obk force-pushed the oli-obk:it_must_be_a_sign branch from 4792b7d to 7d0fcc6 Nov 17, 2019
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 20, 2019

☔️ The latest upstream changes (presumably #66578) made this pull request unmergeable. Please resolve the merge conflicts.

@oli-obk oli-obk force-pushed the oli-obk:it_must_be_a_sign branch from 7d0fcc6 to a329756 Nov 26, 2019
@oli-obk oli-obk marked this pull request as ready for review Nov 26, 2019
Copy link
Member

RalfJung left a comment

LGTM! r=me, but it'd be even better if you could add a test (at least for the sign part) in-tree.

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Dec 3, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 3, 2019

📌 Commit a329756 has been approved by RalfJung

Centril added a commit to Centril/rust that referenced this pull request Dec 3, 2019
Show the sign for signed ops on `exact_div`

r? @RalfJung Cc https://github.com/rust-lang/miri/pull/961/files#r341842128

I'm fairly unhappy with the duplication and the general effort required for this.

Maybe it would be better to add a `display` impl for `ImmTy`?
bors added a commit that referenced this pull request Dec 3, 2019
Rollup of 6 pull requests

Successful merges:

 - #66148 (Show the sign for signed ops on `exact_div`)
 - #66651 (Add `enclosing scope` parameter to `rustc_on_unimplemented`)
 - #66904 (Adding docs for keyword match, move)
 - #66935 (syntax: Unify macro and attribute arguments in AST)
 - #66941 (Remove `ord` lang item)
 - #66967 (Remove hack for top-level or-patterns in match checking)

Failed merges:

r? @ghost
@bors bors merged commit a329756 into rust-lang:master Dec 3, 2019
4 checks passed
4 checks passed
pr #20191126.36 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
@RalfJung RalfJung mentioned this pull request Dec 3, 2019
@oli-obk

This comment has been minimized.

Copy link
Contributor Author

oli-obk commented Dec 3, 2019

Oh... XD I was working locally on deduplicating the rendering logic from this with the one from constants. I'll open a separate PR and that will also have tests for rustc then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.