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

Feature: upper- or lowercase hexadecimal literals #4903

Merged
merged 1 commit into from
Sep 9, 2021

Conversation

ArjenL
Copy link
Contributor

@ArjenL ArjenL commented Jul 15, 2021

A small feature to consistently case hexadecimal literals in your Rust code.

@calebcartwright
Copy link
Member

Apologies for the delay but still haven't had a chance to go through this in detail. It seems like a reasonable feature to support so I suspect we'll be able to move ahead, could just potentially have a bit of bikeshedding to do on implementation specifics.

@ArjenL
Copy link
Contributor Author

ArjenL commented Jul 30, 2021

Thanks! Some bikeshedding is fine, I'm sure some things could be better/more idiomatic.

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

This is in great shape, thanks so much! It does sound like a small feature at first, but very much looking forward to being able to get it released.

Will need a couple minor changes (largely renaming the variants and updating all the refs to them) but should be good to merge afterwards.

Comment on lines 1052 to 1056
Change the case of the letters in hexadecimal literal values

- **Default value**: `Ignore`
- **Possible values**: `ToUpper`, `ToLower`
- **Stable**: No
Copy link
Member

Choose a reason for hiding this comment

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

Couple minor things I'd like to tweak here. Let's adjust the text to reflect the fact that it can set vs. implying it will change.

Instead of Ignore, we should use Preserve to be consistent with variants in our other configuration options, and then let's drop the To prefix on the other variants.

Suggested change
Change the case of the letters in hexadecimal literal values
- **Default value**: `Ignore`
- **Possible values**: `ToUpper`, `ToLower`
- **Stable**: No
Control the case of the letters in hexadecimal literal values
- **Default value**: `Preserve`
- **Possible values**: `Upper`, `Lower`
- **Stable**: No

src/config/mod.rs Outdated Show resolved Hide resolved
src/config/options.rs Outdated Show resolved Hide resolved
src/expr.rs Outdated Show resolved Hide resolved
src/expr.rs Outdated
Comment on lines 1224 to 1233
if let Some(hex_lit) = hex_lit {
return Some(format!(
"0x{}{}",
hex_lit,
lit.token.suffix.map_or(String::new(), |s| s.to_string())
));
}
}

Some(context.snippet(span).to_owned())
Copy link
Member

Choose a reason for hiding this comment

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

Could you make sure the values will continue to be passed through wrap_str as they are currently? If we drop that then we're potentially introducing some breaking formatting changes around some edge cases approaching width limits

tests/source/hex_literal_upper.rs Outdated Show resolved Hide resolved
tests/target/hex_literal_ignore.rs Outdated Show resolved Hide resolved
tests/source/hex_literal_upper.rs Outdated Show resolved Hide resolved
tests/target/hex_literal_lower.rs Outdated Show resolved Hide resolved
tests/target/hex_literal_upper.rs Outdated Show resolved Hide resolved
@calebcartwright
Copy link
Member

Thanks for making these updates, though please note there's still at least one more that needs to be resolved before this can proceed

@calebcartwright
Copy link
Member

Awesome thank you! Going to go ahead and merge this which will put it on track to be included the next time we update rustfmt. No ETA as of yet, but there will be couple PRs to move across branches and ultimately update the submodule in the main compiler repo. GH should notify you on those given the authorship of the commits so you'll know once this hits nightly

@calebcartwright calebcartwright merged commit d304dec into rust-lang:stbu Sep 9, 2021
@calebcartwright calebcartwright added 1x-backport:pending Fixed/resolved in source but not yet backported to a 1x branch and release and removed pr-waiting-on-author labels Sep 15, 2021
@calebcartwright calebcartwright added 1x-backport:completed 2x-port:pending and removed 1x-backport:pending Fixed/resolved in source but not yet backported to a 1x branch and release labels Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants