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.

Sign up for GitHub

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

Jump to bottom

spanned HIR node enums should be named FooKind rather than being named Foo_ and glob-exported #51968

Closed
4 tasks
zackmdavis opened this issue Jul 1, 2018 · 3 comments
Closed
4 tasks

spanned HIR node enums should be named FooKind rather than being named Foo_ and glob-exported #51968

zackmdavis opened this issue Jul 1, 2018 · 3 comments
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.

Comments

@zackmdavis
Copy link
Member

@petrochenkov writes:

The convention enum Something_ { SomethingVariant } is "deprecated" and exists in HIR only because not all HIR structures were updated to the "modern" form enum SomethingKind { Variant } like it was done in AST.

New structures should not use it, so it'd be better to rename Visibility_ to VisibilityKind.

The referenced changes to AST were mostly in February 2016's #31487 (justification). The following HIR nodes are still using the underscore-suffix convention, and should probably be changed to the Kind style for consistency:
@zackmdavis zackmdavis mentioned this issue Jul 1, 2018
Merged
zackmdavis added a commit to zackmdavis/rust that referenced this issue Jul 1, 2018
@zackmdavis
call it hir::VisibilityKind instead of hir::Visibility_:*
43a0a65 
It was pointed out in review that the glob-exported
underscore-suffixed convention for `Spanned` HIR nodes is no longer
preferred: see February 2016's rust-lang#31487 for AST's migration away from
this style towards properly namespaced NodeKind enums.

This concerns rust-lang#51968.
@varkor
Copy link
Member

varkor commented Jul 1, 2018

Presumably this applies to Expr_ too?

@zackmdavis
Copy link
Member Author

Ah, yes: I was searching for Spanned, but the same rationale would apply to Expr_, Ty_, and ForeignItem_.

@oli-obk oli-obk added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Jul 2, 2018
@oli-obk
Copy link
Contributor

oli-obk commented Jul 2, 2018

Since any of these changes will break clippy I would be delighted if a PR doing them would also update clippy with the relevant changes. Not a blocker though!

@Mark-Simulacrum Mark-Simulacrum added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Jul 4, 2018
@csmoe csmoe mentioned this issue Jul 11, 2018
Merged
bors added a commit that referenced this issue Jul 16, 2018
@bors
Auto merge of #52264 - csmoe:kind, r=oli-obk
50702b2 
Rename spanned HIR node enums from Foo_ to FooKind

Closes #51968
r? @oli-obk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@oli-obk @zackmdavis @varkor @Mark-Simulacrum