[Driver] Move some string definitions around (NFC) #4493
Conversation
| }; | ||
|
|
||
| /// \returns A string representation of the given key. | ||
| inline StringRef getName(TopLevelKey Key) { |
There was a problem hiding this comment.
Please mark this static. Non-static inlines have weird rules that we just avoid.
|
I do feel a little weird putting the build record tags in Action.h, which is theoretically a "public" header. Maybe those belong in CompilationRecord.h as well. |
|
But eliminating duplication of magic constants is a good idea. The |
modocache
force-pushed
the
driver-compilation-record
branch
from
8e21ac3 to
4456b93
Compare
|
@jrose-apple Hopefully I've addressed your comments: I moved everything to |
| } | ||
|
|
||
| /// \returns The string identifier used to represent the given status in a | ||
| /// compilation record file (.swiftdeps file). |
There was a problem hiding this comment.
Last nitpick: can you add a note here pointing out that this will not round-trip?
There was a problem hiding this comment.
Ah, I was actually confused when you mentioned that earlier. By "not round-trip", you mean that getInfoStatusForIdentifier(getIdentifierForInputInfoStatus(Status)) != Status, is that right? Since the left-hand side would be an Optional<Status>?
There was a problem hiding this comment.
I added a note to the function below, based on my interpretation of "round trip" 😇
There was a problem hiding this comment.
Ah, no, I mostly meant that there was no dedicated identifier for NewlyAdded, and that in general not every input status will necessarily have a unique compilation record identifier.
There was a problem hiding this comment.
Aha, gotcha. My bad! Updating now.
There was a problem hiding this comment.
Done! Let me know if that's what you had in mind.
There was a problem hiding this comment.
Works for me. Thanks, Brian!
modocache
force-pushed
the
driver-compilation-record
branch
from
4456b93 to
d9ea330
Compare
Define compilation record (.swiftdeps) top-level keys, as well as string identifiers used in compilation record files (like "!dirty" and "!private"), in a single location. NFC.
modocache
force-pushed
the
driver-compilation-record
branch
from
d9ea330 to
2b3647e
Compare
|
@swift-ci Please smoke test |
|
@swift-ci Please smoke test Linux platform |
2 similar comments
|
@swift-ci Please smoke test Linux platform |
|
@swift-ci Please smoke test Linux platform |
|
Looks like all the tests passed, and according to #4585 (comment) my merge permissions have been restored. So I'll go ahead and merge this myself. :) |
What's in this pull request?
I've been poking around what
.swiftdepsfiles are and how they work, and noticed a small amount of duplication, acrossDriver.cppandCompilation.cpp, in some strings used in the files.I'm not sure how much of an improvement this is, but these two commits "centralize" the duplication. YAML keys like
versionandinputsnow exist inCompilationRecord.h. Input status identifiers like!dirtyand!privateare now defined as part of theCompileJobAction::InputInfostruct.Is it worth making the change? I'm not sure. Feel free to close this pull request if you don't think it is. :)
Resolved bug number: None
Before merging this pull request to apple/swift repository:
Triggering Swift CI
The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:
Smoke Testing
A smoke test on macOS does the following:
device standard libraries are not built.
version of these tests are not run.
A smoke test on Linux does the following:
tests are not run.
Validation Testing
Lint Testing
Note: Only members of the Apple organization can trigger swift-ci.