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

Fmt flags refactor #24492

Closed
wants to merge 5 commits into from

Conversation

Projects
None yet
3 participants
@richo
Copy link
Contributor

richo commented Apr 16, 2015

While implementing the fmt::Pointer traits, needing to awkwardly bitshift the variants of an enum everywhere irked me as being very awkward. Considering these values are never passed as arguments, it's not like their membership in the enum actually grants any type safety.

bitflags is not available in core, so I manually created the bitflags. This shouldn't break existing code, since they're not exported, but it seems that they should be, since there doesn't appear to be a blessed way to test for, eg the alternate flag in a formatter in user code.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Apr 16, 2015

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@richo

This comment has been minimized.

Copy link
Contributor Author

richo commented Apr 16, 2015

Err, tbc this is WIP. I had a moment and opened the PR before running the tests, which I have undoubtedly broken. Open to feedback on the spirit of the PR though!

richo added some commits Apr 16, 2015

fmt_macros: Restructure to consume the new enum in fmt
The "enum" in libfmt is only public so that it can be consumed by
libfmt_macros, it should never be directly accessible by an end user.

(Or potentially it should become the blessed accessor)
@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Apr 17, 2015

Due to the unstable internals, this shouldn't need to bother with a V2 flag format. Additionally, I suspect that a change such as this would want to be accompanied with a stabilization of the flags function on Formatter as well, so this may wish to have some broader discussion before moving forward.

@richo

This comment has been minimized.

Copy link
Contributor Author

richo commented Apr 17, 2015

That sounds reasonable (And definitely lines up with my experience once I started getting fmt_macros to work around this). Is the correct move forward an RFC or a thread on internals? I care about this enough to shepard it.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Apr 18, 2015

A thread on internals is probably fine for now, I suspect that the two main options here would be specific is_foo accessors or just a packed bitflags as you've done here, so there's not a whole lot of design space, just want to make sure others are aware!

@richo

This comment has been minimized.

Copy link
Contributor Author

richo commented Apr 18, 2015

@richo richo closed this Apr 18, 2015

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