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

Prefer enum Endian instead of arbitrary String in rustc_target::Target #77604

Closed
wants to merge 4 commits into from

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Oct 6, 2020

Strong type is better than String I think.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 6, 2020
@tesuji
Copy link
Contributor Author

tesuji commented Oct 6, 2020

I'm going to use more enum types for members of Target when it makes sense,
unless people prefer not to.
Discussion (only me currently) could happen in this zulip topic: https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/using.20enum.20for.20members.20of.20rustc_target.3A.3Aspec.3A.3ATarget.20.3F

@tesuji tesuji changed the title Target enumeratePrefer enum Endian instead of arbitrary String in rustc_target::Target Prefer enum Endian instead of arbitrary String in rustc_target::Target Oct 6, 2020
@petrochenkov
Copy link
Contributor

Could you move the enum Endian to compiler\rustc_target\src\spec\mod.rs, or at least reexport it from there, and use imports use crate::spec::Endian instead of full paths like crate::abi::Endian.


pub fn is_little(&self) -> bool {
Self::Little == *self
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you remove the trivial methods is_big and is_little and use comparisons like e == Endian::Little instead?

Copy link
Contributor Author

@tesuji tesuji Oct 7, 2020

Choose a reason for hiding this comment

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

We could, but then we have to import the enum type everywhere
while its only usecase is a member of a struct.
With this boring trivial methods, we don't need import the enum type to check its fieldless variants.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think importing it would probably be better, is it a totally different import?

Copy link
Contributor Author

@tesuji tesuji Oct 7, 2020

Choose a reason for hiding this comment

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

How? If it's used to assign value to a variable, I would import them. Here we only check the property of it.

Also I haven't talked about there are already existing cases in the compiler.

compiler/rustc_target/src/abi/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_target/src/abi/mod.rs Outdated Show resolved Hide resolved
@petrochenkov petrochenkov 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 Oct 6, 2020
@tesuji
Copy link
Contributor Author

tesuji commented Oct 7, 2020

Could you move the enum Endian to compiler\rustc_target\src\spec\mod.rs,

Endian is under abi things seems logical to me.

or at least reexport it from there,
and use imports use crate::spec::Endian instead of full paths like crate::abi::Endian.

There is an MCP to "Ban pub re-exports in compiler",
Also why using crate::spec::Endian instead? I don't see the difference. They are all under the same crate.
Or you want to limit access to abi module when declaring a target ?

@rustbot modify labels: -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot 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 Oct 7, 2020
@petrochenkov
Copy link
Contributor

There is an MCP to "Ban pub re-exports in compiler",

The first specific application of which (#77494) wasn't approved.

Also why using crate::spec::Endian instead?

That's what specific targets mostly import from, imports will look much cleaner.

(The goal here is to clean things up, but now the PR brings inconsistencies into rustc_target code instead, and does something opposite to a cleanup by introducing the micro-methods.)

@petrochenkov petrochenkov 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 Oct 7, 2020
@petrochenkov
Copy link
Contributor

So I had one more though - perhaps endianness should live in TargetOptions instead of Target.
The options in Target are supposed to lack defaults, but target_endian has a very clear default - little endian, so it doesn't need to be explicitly specified by every target.
(That applies to a couple of other options in Target as well.)

@petrochenkov
Copy link
Contributor

So I had one more though - perhaps endianness should live in TargetOptions instead of Target.
(That applies to a couple of other options in Target as well.)

Implemented in #77729, I'll mark this PR as waiting on it.

@petrochenkov petrochenkov added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 8, 2020
@tesuji
Copy link
Contributor Author

tesuji commented Oct 9, 2020

The first specific application of which (#77494) wasn't approved.

Ok, will do.

(The goal here is to clean things up,

Perhaps I didn't make things clear. I've never said this is a clean up. Just that I thought everybody prefers strong type when possible.

now the PR brings inconsistencies into rustc_target code instead, and does something opposite to a cleanup by introducing the micro-methods.)

So does Option::is_some, is_none.
The micro methods are used to make things easier for contributors.
If you disagree, I will remove them, just because you're reviewer.

@est31 est31 mentioned this pull request Oct 15, 2020
@bors
Copy link
Contributor

bors commented Oct 15, 2020

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@bors
Copy link
Contributor

bors commented Nov 8, 2020

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@petrochenkov
Copy link
Contributor

#77729 and #78875 have landed, unblocking.

@petrochenkov petrochenkov removed the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Nov 11, 2020
@petrochenkov petrochenkov added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 11, 2020
@tesuji tesuji closed this Nov 24, 2020
@tesuji tesuji deleted the target-enumerate branch November 24, 2020 11:56
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jan 7, 2021
…nkov

Limit target endian to an enum instead of free string

This is rust-lang#77604 revived.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 7, 2021
Limit target endian to an enum instead of free string

This is rust-lang#77604 revived.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants