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

ast: IdentifierReference's reference_flag is always empty #4512

Closed
Dunqing opened this issue Jul 28, 2024 · 5 comments · Fixed by #5077
Closed

ast: IdentifierReference's reference_flag is always empty #4512

Dunqing opened this issue Jul 28, 2024 · 5 comments · Fixed by #5077
Assignees
Labels
A-ast Area - AST C-bug Category - Bug

Comments

@Dunqing
Copy link
Member

Dunqing commented Jul 28, 2024

SemanticBuilder's already resolved reference usage but never doesn't set reference_flag for IdentifierReference.

Although we can get reference_flag by reference_id, would be great if we could directly reference_flag.

@Dunqing Dunqing added the C-bug Category - Bug label Jul 28, 2024
@Dunqing
Copy link
Member Author

Dunqing commented Jul 28, 2024

SemanticBuilder's already resolved reference usage but never doesn't set reference_flag for IdentifierReference.

I know the reason now. We can only get the immutable IdentifierReference from AstKind, so we can't change the reference_flag. In this way, we need to change reference_flag: ReferenceFlag to reference_flag: Cell<ReferenceFlag>

@Dunqing Dunqing added the A-ast Area - AST label Jul 28, 2024
Boshen pushed a commit that referenced this issue Jul 29, 2024
…Speicifer` by `ReferenceFlags` (#4513)

After #4511. We can determine if a binding is a type binding just by the `ReferenceFlags`.

We can make it even better with `ident.reference_flag` once #4512 is sorted out
@overlookmotel
Copy link
Contributor

overlookmotel commented Jul 29, 2024

Agreed, if we are keeping that field, it should be Cell<ReferenceFlag>. But should we remove it entirely from IdentifierReference? Or remove it from Reference? It doesn't really need to be in 2 places does it? Will make transformer/minifier more complex to have to update in 2 places when flags get changed.

@Dunqing
Copy link
Member Author

Dunqing commented Jul 30, 2024

make transformer/minifier more complex to have to update in 2 places when flags get changed.

Yes! It is better to remove reference_flag from IdentifierReference

@overlookmotel
Copy link
Contributor

Yes! It is better to remove reference_flag from IdentifierReference

I agree that's the better one to remove. It makes more sense for that info to be stored in Reference.

Annoyingly, removing reference_flag from IdentifierReference doesn't save any bytes in AST, as that type has excess padding anyway. Whereas removing it from Reference would reduce size of Reference by 4 bytes.

But still, in my view it's more important that our data structures make logical sense, than that we've optimized every last drop out of them.

@overlookmotel
Copy link
Contributor

Closed in #5077.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ast Area - AST C-bug Category - Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants