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

Type: add helpers for managing typeof and attributed types #757

Closed
wants to merge 15 commits into from

Conversation

ehaas
Copy link
Collaborator

@ehaas ehaas commented Aug 21, 2024

The idea behind this is to prepare for #733 by making typeof and attributed types slightly less annoying to deal with, by moving the recursive call logic to a single place (2 actually), which should make it easier to add a new typedef specifier which will generally just call into its subtype for most of the Type functions.

@Vexu
Copy link
Owner

Vexu commented Aug 22, 2024

Wouldn't something more like canonicalize be better? This strategy already requires a duplicate function with a one arg difference.

Maybe a Type.base() which would use a while loop instead of recursion. Most places that use canonicalize seem like they actually want that kind of functionality.

@ehaas
Copy link
Collaborator Author

ehaas commented Aug 22, 2024

Is the idea that each function that currently recurses on typeof/attributed types, should instead call canonicalize/base on its argument first? I think that would also work and yeah probably simpler.

@Vexu
Copy link
Owner

Vexu commented Aug 23, 2024

Something like that yes. It would also work well if in the future Type is reworked to be interned or something similar.

@ehaas
Copy link
Collaborator Author

ehaas commented Aug 24, 2024

This branch has a fairly heavy performance hit (~5% for large files), and even with just Type.base replacing canonicalize where we only want the specifier, it's still about 1.5% slower. So now I'm kinda leaning toward, we just implement typedef specifiers in the code as-is, and then hopefully clean everything up with Type interning in the future.

@ehaas ehaas closed this Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants