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

[SR-12022] Refactor LiteralExpr subclasses to combine common initializer code paths #54459

Closed
beccadax opened this issue Jan 13, 2020 · 4 comments
Labels
compiler The Swift compiler in itself good first issue Good for newcomers task

Comments

@beccadax
Copy link
Contributor

Previous ID SR-12022
Radar rdar://problem/69697800
Original Reporter @beccadax
Type Task
Status Closed
Resolution Done
Additional Detail from JIRA
Votes 0
Component/s Compiler
Labels Task, StarterBug
Assignee interfere (JIRA)
Priority Medium

md5: ead22ff9893454d26c131a8334ef82dd

Issue Description:

Every *LiteralExpr AST node has an Initializer member variable or equivalent, and a large subset of them have a second BuiltinInitializer member variable. However, each subclass declares its own copies of these. This leads to a lot of unnecessary duplication; for instance, SILGenFunction::emitLiteral() has about a dozen lines that merely call getInitializer() or getBuiltinInitializer() on various subclasses.

You could improve this situation by refactoring the LiteralExpr subclasses:

  1. Rename InterpolatedStringLiteralExpr::get/setResultInit() to get/setInitializer() and update use sites so that it matches the convention of the other literal expressions

  2. Hoist the Initializer member variable and its accessor methods into LiteralExpr

  3. Introduce a new subclass with the BuiltinInitializer member variable and its accessor methods

  4. Make all the literals with {{BuiltinInitializer}}s subclasses of that new class

  5. Audit the call sites of getInitializer(), setInitializer(), getBuiltinInitializer(), and setBuiltinInitializer() to see how much code you can now consolidate

Since you're changing the hierarchy of Expr subclasses, you'll need to update include/swift/AST/ExprNodes.def. The comments at the top of the file should help you do that.

This task should not cause any changes in functionality—it's just a straight-up refactoring and simplification—so if you've done this correctly, all existing tests should pass and you won't need to write any new ones.

@beccadax
Copy link
Contributor Author

@swift-ci create

@swift-ci
Copy link
Collaborator

Comment by Alexey Komnin (JIRA)

I am gonna try to resolve this.

@beccadax
Copy link
Contributor Author

interfere (JIRA User) Great! When your PR is ready, please feel free to ping me on GitHub using @brentdax.

(I’m sorry I haven’t gotten around to reviewing your previous PR—I promise I’m not ignoring it!)

@swift-ci
Copy link
Collaborator

Comment by Alexey Komnin (JIRA)

PR -> #34132

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler The Swift compiler in itself good first issue Good for newcomers task
Projects
None yet
Development

No branches or pull requests

2 participants