Move isFallback to FileBuildSettings #908
Merged
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
ahoppen
reviewed
Oct 18, 2023
Comment on lines
54
to
55
| public func buildSettings(for uri: DocumentURI, language: Language, isFallback: Bool = true) -> FileBuildSettings? { | ||
| var fileBuildSettings: FileBuildSettings? |
There was a problem hiding this comment.
Instead of passing isFallback here, I would prefer to set it to true here, since it is coming from a fallback build system and overriding it to false in BuildSystem if we don’t have a real build system.
There was a problem hiding this comment.
right, by doing so, the func buildSettings in FallbackBuildSystem would be more clean.
| @@ -25,9 +25,13 @@ public struct FileBuildSettings: Equatable { | |||
| /// The working directory to resolve any relative paths in `compilerArguments`. | |||
| public var workingDirectory: String? = nil | |||
|
|
|||
| public init(compilerArguments: [String], workingDirectory: String? = nil) { | |||
| /// The isFallback to know if the fileBuildSetting is specified or the default one. | |||
There was a problem hiding this comment.
I would phrase this slightly differently.
Suggested change
| /// The isFallback to know if the fileBuildSetting is specified or the default one. | |
| /// Whether the build settings were computed from a real build system or whether they are synthesized fallback arguments while the build system is still busy computing build settings. |
ahoppen
reviewed
Oct 19, 2023
Comment on lines
112
to
133
| // If there is no build system and we only have the fallback build system, | ||
| // we will never get real build settings. Consider the build settings | ||
| // non-fallback. |
There was a problem hiding this comment.
This comment now applies to the settings.isFallback = buildSystem != nil line, which, just to make it even more readable would write as
if buildSystem == nil {
// If there is no build system and we only have the fallback build system,
// we will never get real build settings. Consider the build settings
// non-fallback.
settings.isFallback = false
}There was a problem hiding this comment.
agree, let me update the code.
joehsieh
force-pushed
the
move-isFallback-to-FileBuildSettings
branch
from
cd1a02b to
90c4f2d
Compare
joehsieh
force-pushed
the
move-isFallback-to-FileBuildSettings
branch
from
90c4f2d to
98ba3ee
Compare
joehsieh
commented
Oct 20, 2023
Comment on lines
+161
to
+178
| settings = settings.patching(newFile: document.pseudoPath, originalFile: mainFile.pseudoPath) | ||
| } | ||
| if let buildSettings { | ||
| let log = """ | ||
| Compiler Arguments: | ||
| \(buildSettings.buildSettings.compilerArguments.joined(separator: "\n")) | ||
| let log = """ | ||
| Compiler Arguments: | ||
| \(settings.compilerArguments.joined(separator: "\n")) | ||
|
|
||
| Working directory: | ||
| \(buildSettings.buildSettings.workingDirectory ?? "<nil>") | ||
| """ | ||
| Working directory: | ||
| \(settings.workingDirectory ?? "<nil>") | ||
| """ | ||
|
|
||
| let chunks = splitLongMultilineMessage(message: log, maxChunkSize: 800) | ||
| for (index, chunk) in chunks.enumerated() { | ||
| logger.log( | ||
| """ | ||
| Build settings for \(document.forLogging) (\(index + 1)/\(chunks.count)) | ||
| \(chunk) | ||
| """ | ||
| ) | ||
| } | ||
| let chunks = splitLongMultilineMessage(message: log, maxChunkSize: 800) | ||
| for (index, chunk) in chunks.enumerated() { | ||
| logger.log( | ||
| """ | ||
| Build settings for \(document.forLogging) (\(index + 1)/\(chunks.count)) | ||
| \(chunk) | ||
| """ | ||
| ) |
There was a problem hiding this comment.
note: this part is a new change for solving the conflicts after rebasing.
ahoppen
approved these changes
Oct 20, 2023
|
@swift-ci Please test |
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.
A refactoring to move isFallback into FileBuildSettings
issue: #861