[SE-0274] Update concise #file implementation in response to first round of review #29412
Conversation
|
@CodaFi This set of changes works, but I'm not very happy with where this code lives in the compiler or how it ends up being called. Do you have any suggestions to improve it? |
|
@swift-ci test |
|
@swift-ci test Windows platform |
beccadax
force-pushed
the
two-revisions-for-slashing
branch
from
81a9927 to
54b93a8
Compare
A proof of concept for tools providing this information in more useful forms.
beccadax
force-pushed
the
two-revisions-for-slashing
branch
from
54b93a8 to
8c01e59
Compare
|
@swift-ci please test |
|
@swift-ci please test Windows platform |
|
@swift-ci please test Windows platform |
8 similar comments
|
@swift-ci please test Windows platform |
|
@swift-ci please test Windows platform |
|
@swift-ci please test Windows platform |
|
@swift-ci please test Windows platform |
|
@swift-ci please test Windows platform |
|
@swift-ci please test Windows platform |
|
@swift-ci please test Windows platform |
|
@swift-ci please test Windows platform |
GYB opens its inputs and outputs in text mode. This is a problem because the recommendation in Windows is to check everything out in LF mode to avoid breaking tests, so gybbing a file ends up converting LFs to CRLFs. That broke test/SILGen/magic_identifier_file_conflicting.swift.gyb. Modify GYB to open its files in binary mode instead of text mode. This is a no-op everywhere but Windows. Note that this change is incomplete: it’s somewhat difficult to switch stdin and stdout to binary mode in Python 2. I’ve added FIXME comments about this.
beccadax
force-pushed
the
two-revisions-for-slashing
branch
from
84b1ef7 to
62ab3df
Compare
|
@swift-ci please test |
|
@swift-ci please test windows platform |
|
@swift-ci please test |
|
@swift-ci please test windows platform |
1 similar comment
|
@swift-ci please test windows platform |
| /// file. | ||
| llvm::SmallVector<Located<StringRef>, 0> VirtualFilenames; | ||
|
|
||
| llvm::StringMap<SourceFilePathInfo> getInfoForUsedFilePaths() const; |
There was a problem hiding this comment.
Could you add a brief doc comment explaining that this returns info for the physical and virtual file paths introduced by this file?
| @@ -321,6 +321,12 @@ class SourceFile final : public FileUnit { | |||
| /// forwarded on to IRGen. | |||
| ASTStage_t ASTStage = Unprocessed; | |||
|
|
|||
| /// Virtual filenames declared by #sourceLocation(file:) directives in this | |||
| /// file. | |||
| llvm::SmallVector<Located<StringRef>, 0> VirtualFilenames; | |||
There was a problem hiding this comment.
It might be worth renaming this to VirtualFilePaths to clarify that it deals with the whole path passed to #sourceLocation, and cannot be used in general to subscript e.g getInfoForUsedFileNames.
There was a problem hiding this comment.
Also, unrelated to this PR, but I think we should also consider renaming SourceFile::getFilename.
| @@ -131,6 +131,9 @@ class LLVM_LIBRARY_VISIBILITY SILGenModule : public ASTVisitor<SILGenModule> { | |||
|
|
|||
| ASTContext &getASTContext() { return M.getASTContext(); } | |||
|
|
|||
| llvm::StringMap<std::pair<std::string, /*isWinner=*/bool>> | |||
| MagicFileStringsByFilePath; | |||
There was a problem hiding this comment.
Nit: I believe MagicFileStringsByFilePath should be indented
This addresses some late-breaking review comments left by @hamishknight on swiftlang#29412.
This PR addresses several issues that came up in the first review of SE-0274:
The format of the
#filestring is now<module>/<filename>. (Technically, the proposal will also allow it to be<module>/<other-stuff>/<filename>instead, but that's for future expansion.)If a
#sourceLocation(file:line:)directive maps to the same#filestring as a real file or another#sourceLocationdirective, but the paths don't match exactly, the compiler now emits a warning.-emit-siland related textual SIL outputs now include a comment with a table mapping#filestrings to#filePathstrings. This is a proof-of-concept demonstrating that we can expose this information in e.g. SourceKit later on.The proposal will be updated to include these changes shortly.