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

Jump to bottom

[SR-13245] Refactor construction of ModuleDecl::ImportFilters to use new initializer list constructor #55685

Closed
typesanitizer opened this issue Jul 17, 2020 · 12 comments
Closed

[SR-13245] Refactor construction of ModuleDecl::ImportFilters to use new initializer list constructor #55685

typesanitizer opened this issue Jul 17, 2020 · 12 comments
Labels
compiler The Swift compiler in itself good first issue Good for newcomers task

Comments

@typesanitizer
Copy link

Previous ID SR-13245
Radar rdar://problem/65747827
Original Reporter @typesanitizer
Type Task
Status Closed
Resolution Done
Additional Detail from JIRA
Votes 0
Component/s Compiler
Labels Task, StarterBug
Assignee None
Priority Medium

md5: dc041543501cdfb3045d66f6f0b1cbb0

Issue Description:

Throughout the codebase, we have patterns like:

  ModuleDecl::ImportFilter filter = ModuleDecl::ImportFilterKind::Public;
  filter |= ModuleDecl::ImportFilterKind::Private;
  filter |= ModuleDecl::ImportFilterKind::SPIAccessControl;

However, this would be cleaner if we used the newly introduced initializer list constructor.

ModuleDecl::ImportFilter filter = {
  ModuleDecl::ImportFilterKind::Public,
  ModuleDecl::ImportFilterKind::Private,
  ModuleDecl::ImportFilterKind::SPIAccessControl
};

The idea would be to search the code for ImportFilterKind:: or similar and see places manually using the |= construction and use the new constructor instead. This shouldn't require any new test cases.
@typesanitizer
Copy link
Author

This should be a good starter task, as the behavior after the refactoring should not change. There is no new functionality to be added. The main challenge would be to get the compiler building properly.

Please feel free to ask for help or code review (I'm varungandhi-apple on GitHub). 🙂

@typesanitizer
Copy link
Author

@swift-ci create

@swift-ci
Copy link
Collaborator

Comment by Nikhil (JIRA)

theindigamer (JIRA User)..will work on this.

@typesanitizer
Copy link
Author

Sure thing! Let me know if you hit any issues.

@swift-ci
Copy link
Collaborator

Comment by Jaz Garewal (JIRA)

srinikhil07 (JIRA User) theindigamer (JIRA User) It looks like some of the changes have been made on this issue. Is it okay if I take on handling the rest or are you still on making changes srinikhil07 (JIRA User)?

@swift-ci
Copy link
Collaborator

Comment by Nikhil (JIRA)

jazgarewal (JIRA User) No, I am not making any changes and I thought the changes made were enough to close this issue.

@swift-ci
Copy link
Collaborator

Comment by Jaz Garewal (JIRA)

srinikhil07 (JIRA User) I might be misreading the issue, but I found other elements that are implementing ImportFilterKind:: lists, for example in SwiftASTContext.cpp, in

SwiftASTContext::CacheUserImports(SwiftASTContext &swift_ast_context,
SymbolContext&sc,
ExecutionContextScope&exe_scope,
lldb::StackFrameWP&stack_frame_wp,
swift::SourceFile&source_file,
Status&error)

at around line 8265, should swift::ModuleDecl::ImportFilter be implemented with the new constructor?

@swift-ci
Copy link
Collaborator

Comment by Nikhil (JIRA)

jazgarewal (JIRA User)

Seems like this is in LLVM, please check with theindigamer (JIRA User)

I made changes only in Swift repo.

@typesanitizer
Copy link
Author

Yeah, that part is code in LLDB. Since OptionSet is a type defined in Swift, I think it is reasonable to change other usage sites as well. jazgarewal (JIRA User) you can submit a PR to apple/llvm-project against the swift/master branch and @ me to review.

@swift-ci
Copy link
Collaborator

Comment by Jaz Garewal (JIRA)

theindigamer (JIRA User) Will do - thank you!

@swift-ci
Copy link
Collaborator

Comment by Jaz Garewal (JIRA)

theindigamer (JIRA User) The PR is ready to go and is at [apple/llvm-project#1662]

@typesanitizer
Copy link
Author

Resolved by #33250 and apple/llvm-project#1662

@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
Assignees
No one assigned
Labels
compiler The Swift compiler in itself good first issue Good for newcomers task
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@typesanitizer @swift-ci