Add progress reports for bridging header compilation. #8103
Conversation
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
to report Size/Align info and continue with the Swift language runtime. This would otherwise cause a fallback to SwiftASTContext and a TypeSystemSwiftTypeRef/SwiftASTContext verification error.
…vm#79912) The `total` parameter for the constructor for Progress was changed to a std::optional in llvm#77547. It was originally set to 1 to indicate non-determinisitic progress, but this commit changes this. First, `UINT64_MAX` will again be used for non-deterministic progress, and `Progress` now has a static variable set to this value so that we can use this instead of a magic number. The member variable `m_total` could be changed to a std::optional as well, but this means that the `ProgressEventData::GetTotal()` (which is used for the public API) would either need to return a std::optional value or it would return some specific integer to represent non-deterministic progress if `m_total` is std::nullopt. (cherry picked from commit 733b86d)
adrian-prantl
requested review from
chelcassanova,
medismailben and
JDevlieghere
|
test with swiftlang/swift#71318 |
kastiglione
approved these changes
Feb 1, 2024
chelcassanova
approved these changes
Feb 1, 2024
medismailben
reviewed
Feb 2, 2024
| : module_name.str())); | ||
| [&progress](llvm::StringRef module_name, | ||
| swift::ASTContext::ModuleImportKind kind) { | ||
| progress.Increment(1, (kind == swift::ASTContext::Overlay |
There was a problem hiding this comment.
Should we have a switch here now that we have 3 kinds of module imports ?
There was a problem hiding this comment.
Honestly I think even the overlay case makes no sense here since the stdlib is always a pure Swift module. I just left it in because it was there before.
| : module_name.str())); | ||
| [&progress](llvm::StringRef module_name, | ||
| swift::ASTContext::ModuleImportKind kind) { | ||
| progress.Increment(1, (kind == swift::ASTContext::Overlay |
Comment on lines
+3642
to
+3651
| case swift::ASTContext::Module: | ||
| progress.Increment(1, module_name.str()); | ||
| break; | ||
| case swift::ASTContext::Overlay: | ||
| progress.Increment(1, module_name.str() + " (overlay)"); | ||
| break; | ||
| case swift::ASTContext::BridgingHeader: | ||
| progress.Increment(1, | ||
| "Compiling bridging header: " + module_name.str()); | ||
| break; |
There was a problem hiding this comment.
In the swift::ASTContext::Module case, we only put the name of the module, if it's an overlay, we show both the name and (overlay) annotation, but if it's a bridging header, we now show Compiling bridging header: with the module name.
I don't have any preference but let's stay consistent across all the Increment updated_detail string.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Requires swiftlang/swift#71318
I needed to cherry-pick a dependency from upstream and also make a drive-by fix in TypeSystemSwiftTypeRef so the test doesn't assert.