Skip to content

[NFC] Clarify import filtering logic and naming. #33986

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

Conversation

varungandhi-apple
Copy link
Contributor

Ran into surprising behavior with SourceFile::getImportedModules when working on #33980, figured I should land this first.

I don't understand why ModuleFile::getImportedModules doesn't have handling for SPI/ShadowedByCrossImportOverlay though, so I haven't touched it.

@varungandhi-apple
Copy link
Contributor Author

@swift-ci test

@varungandhi-apple
Copy link
Contributor Author

swiftlang/llvm-project#1819

@swift-ci please test

@varungandhi-apple
Copy link
Contributor Author

@swift-ci please clean test Windows

@varungandhi-apple
Copy link
Contributor Author

swiftlang/llvm-project#1819

@swift-ci please clean test Windows

@varungandhi-apple
Copy link
Contributor Author

The Windows failure is because Windows doesn't support cross-repository testing.

Copy link
Contributor

@xymus xymus left a comment

Choose a reason for hiding this comment

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

I was convinced at the first s/Public/Exported/. Thanks!

Copy link
Contributor

@beccadax beccadax left a comment

Choose a reason for hiding this comment

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

A definite improvement. (I think it’s this way because there was some thought that @_exported import might eventually be spelled public import when it became a supported language feature. But if that ever happens, we can cross that bridge when we get to it.)

Copy link
Contributor

@hamishknight hamishknight left a comment

Choose a reason for hiding this comment

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

Nice!

@varungandhi-apple varungandhi-apple changed the base branch from master to main September 23, 2020 17:49
The lack of clarity manifested as unexpected behavior when using
getImportedModules to create the module import graph. The new behavior
makes SPI-ness and Shadowing-ness behave similarly in terms of
filtering. We also check if a filter is well-formed to avoid
accidental empty import lists.
@varungandhi-apple
Copy link
Contributor Author

swiftlang/llvm-project#1819

@swift-ci please test

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.

4 participants