Skip to content

Alphabetize typed_insts.h #5401

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

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

geoffromer
Copy link
Contributor

As requested here.

@geoffromer geoffromer requested a review from zygoloid May 1, 2025 23:15
@github-actions github-actions bot requested a review from jonmeow May 1, 2025 23:15
@geoffromer
Copy link
Contributor Author

I'm not sure this is a net improvement, to be honest. In particular, in some cases the concrete inst kinds rely on an Any inst for their member documentation, and this makes it substantially harder to refer to that documentation.

@jonmeow jonmeow removed their request for review May 1, 2025 23:37
Comment on lines +152 to +153
// Common representation for various `*binding_pattern` nodes.
struct AnyBindingPattern {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I hadn't thought about also alphabetizing the categories (previously they'd been immediately before their first member), but I think this is probably appropriate. In addition, I'd be inclined to move them out to a separate file. I don't remember if we already have something in place to check that a category is compatible with all of its members, but maybe now is the time to add such a check if we don't have it already.

@jonmeow
Copy link
Contributor

jonmeow commented May 1, 2025

FWIW, I'm fine with this (slight preference for lexicographic sorting because it's easier to find things, agreeing it's not a big win in this case). But I'm happy to let zygoloid review.

And I pretty much agree that splitting files might be helpful towards a better end result.

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.

3 participants