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

oli-obk · 2019-11-06T08:59:06Z

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 · 2019-11-06T08:59:09Z

⚠️ Warning ⚠️

These commits modify submodules.

Centril · 2019-11-06T09:05:03Z

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 · 2019-11-06T11:12:14Z

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 · 2019-11-06T11:18:15Z

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 · 2019-11-06T11:19:44Z

@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 · 2019-11-09T17:05:08Z

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

JohnCSimon · 2019-11-16T23:48:21Z

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

bors · 2019-11-20T22:33:56Z

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

RalfJung

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

RalfJung · 2019-12-03T08:23:56Z

@bors r+

bors · 2019-12-03T08:23:57Z

📌 Commit a329756 has been approved by RalfJung

@RalfJung

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`?

@ghost

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

oli-obk · 2019-12-03T23:41:13Z

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

rust-highfive assigned RalfJung Nov 6, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 6, 2019

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 16, 2019

oli-obk force-pushed the it_must_be_a_sign branch from 4792b7d to 7d0fcc6 Compare November 17, 2019 13:30

Show the sign of signed interpreter values

a329756

oli-obk force-pushed the it_must_be_a_sign branch from 7d0fcc6 to a329756 Compare November 26, 2019 13:03

oli-obk marked this pull request as ready for review November 26, 2019 13:03

oli-obk added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 26, 2019

RalfJung approved these changes Dec 2, 2019

View reviewed changes

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 Dec 3, 2019

Centril mentioned this pull request Dec 3, 2019

Rollup of 6 pull requests #66982

Merged

bors merged commit a329756 into rust-lang:master Dec 3, 2019

RalfJung mentioned this pull request Dec 3, 2019

update Miri #66925

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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