Skip to content

[AST] Rewrite collectExportedImports() to be non-recursive. #74812

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

Merged
merged 1 commit into from
Jul 1, 2024

Conversation

al45tair
Copy link
Contributor

The issue with recursion here is that if there are enough modules involved, this function will blow the process stack, particularly in the case where the FileUnits are not SourceFiles, since in that instance a SmallVector gets allocated on the stack for each level of the recursion.

rdar://130527640

The issue with recursion here is that if there are enough modules
involved, this function will blow the process stack, particularly
in the case where the `FileUnit`s are not `SourceFile`s, since in
that instance a `SmallVector` gets allocated on the stack for each
level of the recursion.

rdar://130527640
@al45tair
Copy link
Contributor Author

@swift-ci Please test

@@ -1535,31 +1535,36 @@ void ModuleDecl::ImportCollector::collect(
}

static void
collectExportedImports(const ModuleDecl *module,
collectExportedImports(const ModuleDecl *topLevelModule,
Copy link
Contributor

Choose a reason for hiding this comment

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

This entire function is also just duplicating existing code in lib/AST/ImportCache.cpp.

Copy link
Contributor

@daniel-grumberg daniel-grumberg left a comment

Choose a reason for hiding this comment

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

LGTM, sorry about the original recursive variant, I never thought it could blow the process stack.

@al45tair al45tair merged commit 84a0779 into swiftlang:main Jul 1, 2024
5 checks passed
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.

3 participants