Use 'Incremental Imports' Terminology #523
Conversation
This also requires an inversion of polarity to better match the interface expected by the Swift frontend, which has incremental imports enabled by default. We therefore consider the "disable" form to be the positive, and the "enable" form to be the negative with respect to feature enablement.
|
@swift-ci test |
| negative: .enableIncrementalImports, | ||
| default: false) { | ||
| options.formUnion(.disableIncrementalImports) | ||
| } else { | ||
| options.formUnion(.readPriorsFromModuleDependencyGraph) |
There was a problem hiding this comment.
@davidungar If you'd like, I can also flip the polarity on readPriorsFromModuleDependencyGraph to be something like readPriorsFromSourceFileDependencyGraphs
There was a problem hiding this comment.
Thanks for tackling this. I think there is one critical issue:
let enablingOrDisabling = options.contains(.disableIncrementalImports)
And in a few places, IMO it would be clearer to use the bit for the positive, even if the positive is the default. (You'll see it in my comments). But that's not an essential change.
| let enablingOrDisabling = options.contains(.enableCrossModuleIncrementalBuild) | ||
| ? "Enabling" | ||
| : "Disabling" | ||
| let enablingOrDisabling = options.contains(.disableIncrementalImports) |
There was a problem hiding this comment.
What if options contains both?
There was a problem hiding this comment.
These are the bitflags, not the parsed options.
| /// Disables the cross-module incremental build infrastructure by forcing | ||
| /// the incremental build to ignore dependency information embedded in | ||
| /// swiftmodule files. | ||
| /// | ||
| /// FIXME: This option is transitory. We intend to make this the | ||
| /// default behavior. This option should flip to a "disable" bit after that. | ||
| public static let enableCrossModuleIncrementalBuild = Options(rawValue: 1 << 4) | ||
| /// Using this flag pessimizes the incremental compilation session by forcing | ||
| /// the driver to only consider changes to the modification time of imported | ||
| /// swiftmodules, which necessarily requires cascading rebuilds. | ||
| public static let disableIncrementalImports = Options(rawValue: 1 << 4) |
There was a problem hiding this comment.
Should there be a similar comment-rewording for the enable flag?
There was a problem hiding this comment.
I'm not sure what you mean here. The opposite of this flag is Options(rawValue: ~(1 << 4))
| @@ -69,7 +69,7 @@ extension IncrementalCompilationState { | |||
| self.readPriorsFromModuleDependencyGraph = maybeBuildRecord != nil && | |||
| options.contains(.readPriorsFromModuleDependencyGraph) | |||
| self.alwaysRebuildDependents = options.contains(.alwaysRebuildDependents) | |||
| self.isCrossModuleIncrementalBuildEnabled = options.contains(.enableCrossModuleIncrementalBuild) | |||
| self.areIncrementalImportsDisabled = options.contains(.disableIncrementalImports) | |||
There was a problem hiding this comment.
Nit: I tend to favor the bit being true when the feature is enabled.
| if self.parsedOptions.hasFlag(positive: .disableIncrementalImports, | ||
| negative: .enableIncrementalImports, | ||
| default: false) { |
There was a problem hiding this comment.
Wouldn't it be clearer to write positive: .enableIncrementalImports, negative: .disableIncrementalImports, default: true
|
Will try to re-invert this bit but use terminology that isn't enable/disable to help. |
Resync the options flag and drop the experimental cross-module incremental build terminology. The flags are sync'd to ToT Swift at swiftlang/swift#36254.