-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[Dependency Scanning] Update Swift Interface Module's Output Path after vfs
Pruning
#79297
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
artemcm
merged 8 commits into
swiftlang:main
from
qiongsiwu:upstream_rehash_after_vfs_overlay_prune
Feb 14, 2025
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
8055c1d
Initial commit of InterfaceModuleNameExpander implmentation. All test…
qiongsiwu cd6066d
Refactoring swift interface module's output path calculation to use n…
qiongsiwu 15726fc
Adding a test.
qiongsiwu 1ce6949
Address code review comments.
qiongsiwu 9a5a83a
Modify ModuleDependencyInfo so we can set swift interface module outp…
qiongsiwu 69e88b1
Removing unused functions.
qiongsiwu b7b2795
Addressing code review. Removed the helper class, hence we do not nee…
qiongsiwu 23e863d
Address code review: adding a test for -experimental-clang-importer-d…
qiongsiwu File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel you want to make to two separate function call here, especially when nothing is pruned, there is no need to go through and update everything.
The rough break down for how the updating process works is
finalize()
will update the module info to avoid repeated updating for the command-line for minor changes since that is not efficient. Everything before that is information collecting to help the finalize function to do the final update, and Resolver class itself has private fields to hold the information collected.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can fold
updateSwiftInterfaceModuleOutputPath
intopruneUnusedVFSOverlay
to avoid two function calls. I am a bit confused by the commentwhen nothing is pruned, there is no need to go through and update everything.
I think in the case of swift interface modules, we no longer set its output path and context hash until here (relevant discussion). My understanding is that we want to update the command line only after we compute the correct output path, whether we have some options to prune or not. This way we don't end up with a path that can potentially points to nothing. Could you help me understand in which cases I can avoid the update?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be confused a little bit when I review the code because I thought the function is adding the output path to the command-line. I am a bit confused who is in charged of updating the
-o
flag on the command-line for building interface.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that this is where we compute the output path and add it to the command line.
swift/lib/Serialization/ScanningLoaders.cpp
Line 215 in 8c8d573
I think after this PR lands, I shall refactor this code. I don't think we need to calculate
-o
command here. We can directly calculate the output path when we are in the resolver.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Posted #79752 to refactor how we add
-o
to the command line.