fmt::Pointer padding #24186
fmt::Pointer padding #24186
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
This isn't the behavior that I would personally expect from these routines, could you explain a little bit more about why you'd like to make this change? |
|
Yeah, so this morning I did a little more poking, and discovered that I was misremembering what In general, I find that 99% of the time I'm printing a pointer is because I'll copypaste it elsewhere (generally a python repl), do some math on it, then do something with it elsewhere. Given this workflow, padding them all to the native pointer width of the platform cases them to line up neatly, make it easy to eyeball if you have a bunch of values all on page boundaries, etc. The rationale for the change is pretty much entirely that I would assume this will only be used in debugging, and for that usecase, padded pointers seem much more user friendly to me. |
|
With the debug builders work @sfackler has been doing the |
|
My personal preference would be this, but making |
|
It should actually just be as simple as checking for |
|
Oh, amazing. I did wonder why the current code sets I'll update the PR when I get a sec. EDIT: While I'm asking questions, do I need to unfrob the flags on the way back out? It wasn't very clear to me. |
richo
force-pushed
the
pad-pointers
branch
4 times, most recently
from
335405d to
a56ea72
Compare
|
I think this is good to go, r? @alexcrichton Preempting your nit, the body of the Pointer formatter is kinda awkward. I considered adding a PrefixBaseChar flag, which would avoid needing to hijack Alternative. Happy to do that if you think it's worthwhile. |
| #[cfg(target_pointer_width = "32")] | ||
| const POINTER_PADDING: Option<usize> = Some(10); | ||
| #[cfg(target_pointer_width = "64")] | ||
| const POINTER_PADDING: Option<usize> = Some(18); |
There was a problem hiding this comment.
Wherever possible I tend to prefer if cfg!(...) over #[cfg], and it looks like for all uses of #[cfg] here the cfg!-based form would suffice?
There was a problem hiding this comment.
Sure. I guess the only correctness issue with it is that right now, if you build on a platform that doesn't have 32 or 64 bit wide pointers, it will explode violently.
Granted, there's no obvious sign of anyone trying that soon, but the most obvious cfg! based construction will silently skip a function call on such a platform. How defensive is it reaonable to be?
There was a problem hiding this comment.
I'm more worried that #[cfg] code isn't typechecked at all if it's for a different platform, often leading to really annoying cross-platform bugs.
| #[cfg(target_pointer_width = "32")] | ||
| const PTR: &'static str = "0x00001234"; | ||
| #[cfg(target_pointer_width = "64")] | ||
| const PTR: &'static str = "0x0000000000001234"; |
|
Ok, think we're finally good to go 🎉 |
This pads out the printing of pointers to their native width. Extracted from and rebased on top of #24144
|
💔 Test failed - auto-mac-32-opt |
|
@bors: retry On Fri, Apr 10, 2015 at 4:49 PM, bors notifications@github.com wrote:
|
This pads out the printing of pointers to their native width. Extracted from and rebased on top of #24144
This pads out the printing of pointers to their native width.
Extracted from and rebased on top of #24144