Frontend: use preferred path spellings for paths #81103
Conversation
|
@swift-ci please smoke test |
|
@swift-ci please smoke test macOS platform |
|
CC: @j-hui |
There was a problem hiding this comment.
Thanks for getting around to this. I think this is the right thing to do here (and avoids the lifetime issues I encountered in #80564).
However, I'm not convinced that this will close #81012 just yet because it only canonicalizes the path separators from -verify-additional-file. My logging showed that the paths from -verify-additional-file and from the capture error both have inconsistent path separators:
# .---command stderr------------
# | Got additional file: [[42]]::C:\Users\swift-ci\jenkins\workspace\swift-PR-windows\swift\test\Interop\Cxx\foreign-reference/Inputs/inheritance.h
# | Captured error in [[2]]::C:\Users\swift-ci\jenkins\workspace\swift-PR-windows\swift\test\Interop\Cxx\foreign-reference/Inputs\inheritance.h: 125
…
So, this patch should fix the problem for bufID [[42]] here, but not bufID [[2]].
|
@swift-ci please smoke test |
|
I think that fixing |
|
@swift-ci please test |
|
@swift-ci please test Windows platform |
compnerd
requested review from
zoecarver,
egorzhdan,
beccadax,
ian-twilightcoder,
Xazax-hun,
fahadnayyar and
susmonteiro
as code owners
|
@swift-ci please smoke test |
| @@ -934,7 +934,9 @@ importer::addCommonInvocationArguments( | |||
| invocationArgStrs.push_back("-isystem"); | |||
| invocationArgStrs.push_back(path.Path); | |||
| } else { | |||
| invocationArgStrs.push_back("-I" + path.Path); | |||
| llvm::SmallString<261> Path{path.Path}; | |||
| llvm::sys::path::make_preferred(Path); | |||
There was a problem hiding this comment.
Don't we need to do this for the path strings above too?
There was a problem hiding this comment.
Yes, I missed that in this revision, but I didn't want to interrupt the build itself.
| auto bufferID = SM.getIDForBufferIdentifier(path); | ||
| llvm::SmallString<261> buffer{path}; | ||
| llvm::sys::path::make_preferred(buffer); | ||
| auto bufferID = SM.getIDForBufferIdentifier(buffer); |
There was a problem hiding this comment.
Should there be assertions in central locations like getIDForBufferIdentifier() to catch code that uses non-preferred paths? Or maybe a new type or something? I honestly find it a little difficult to believe that there are only four places where we need to do this.
There was a problem hiding this comment.
I don't know what that central location is, this has been a long standing issue. This fixes the issues that are being uncovered here by enabling some of the tests that you added a long time ago.
|
@swift-ci please smoke test |
|
@swift-ci please test Windows platform |
|
@swift-ci please test Windows platform |
Convert paths to the preferred spelling before passing it to the SourceManager to ensure that we have the canonical spelling. This is important as a path with a non-canonical path separator would otherwise fail to be looked up in the source manager buffer.
compnerd
requested review from
xymus,
eeckstein,
AnthonyLatsis,
rjmccall and
adrian-prantl
as code owners
|
@swift-ci please test Windows platform |
Convert paths to the preferred spelling before passing it to the SourceManager to ensure that we have the canonical spelling. This is important as a path with a non-canonical path separator would otherwise fail to be looked up in the source manager buffer.