[swift-dev] Filename, Line and Column for SourceLoc

[rintaro ishizaki]

Thanks!

I'd go with "presumed".

I've just started a WIP pull request.
https://github.com/apple/swift/pull/8203


2017-03-17 7:37 GMT+09:00 Ben Langmuir <blangmuir at apple.com>:

> +1.  The fact that APIs named “getLineNumber” and “getLineAndColumn” do
> subtly different things is pretty awful.
>
> I like “presumed” better than “virtual”, but it’s not a big deal to me
> either way.
>
> Ben
>
>
> On Mar 16, 2017, at 3:20 PM, Jordan Rose via swift-dev <
> swift-dev at swift.org> wrote:
>
> FWIW this sounds like a great idea to me. Clang calls these "presumed
> locations" instead of "virtual locations" but either way is fine with me.
> I'd like one of the SourceKit folks to weigh in, though, since they're the
> most involved in operations at this layer. (That'd be Argyrios Kyrtzidis,
> Ben Langmuir, Xi Ge, David Farler, or Nathan Hawes.)
>
> Jordan
>
>
> On Mar 15, 2017, at 23:23, rintaro ishizaki via swift-dev <
> swift-dev at swift.org> wrote:
>
> Hi all,
>
> I would like to propose to reorganize methods for retrieving filename,
> line and column from SourceLoc.
>
> *Motiavion*
>
> When we want to retrieve filename, line and column from SourceLoc, we
> have several ways to do that.
>
> Filename:
>   SourceManager::getBufferIdentifierForLoc
>   SourceManager::getIdentifierForBuffer
>   SourceFile::getFilename
>   LoadedFile::getFilename
>   llvm::MemoryBuffer::getBufferIdentifier
>
> Line and Column:
>   SourceManager::getLineAndColumn
>   SourceManager::getLineNumber
>
> Some of them respect #sourceLocation directive, but some not:
>
> Virtual (respect #sourceLocation):
>   SourceManager::getBufferIdentifierForLoc
>   SourceManager::getLineAndColumn
>
> Actual:
>   SourceManager::getIdentifierForBuffer
>   SourceFile::getFilename
>   LoadedFile::getFilename
>   llvm::MemoryBuffer::getBufferIdentifier
>   SourceManager::getLineNumber
>
> Mixed use of them causes errors like https://github.com/apple/
> swift/pull/5425
> Since current method namings are unclear about this, I think it's very
> error prone. Also, we currently don't have any method for *real*
> getLineAndColumn. When we want to retrieve the real line and column, we
> have use getLineNumber(loc) for the line, and getLineAndColumn(loc).second
> for the column. Most of our codebase doesn't bother to do that, though.
>
> I suspect that Xcode crash when using #sourceLocation in PlayGround is
> probably caused by this kind of problem; i.e. Xcode expects "real" line and
> column, but the compiler or sourcekit returns virtual one.
>
>
> *Proposed Solution*
>
> *Reorganize SourceManager methods.*
>
> We need methods for "real" one, and "virtual" one. And they need to have
> distinct name for that.
>
> ** RFC for method names **
>
> class SourceManager {
>
> public:
>
>   /// @{
>   /// Real buffer-name, line, and column.
>   /// These methods does not respect '#sourceLocation'
>
>   /// Returns the identifier string for the buffer with the given ID.
>   StringRef
>   getIdentifierForBuffer(unsigned BufferID) const;
>
>   /// Returns the buffer identifier string for the Loc.
>   /// If 'BufferID' is provided 'Loc' must come from that buffer.
>   StringRef
>   getIdentifierForBuffer(SourceLoc Loc, unsigned BufferID = 0) const;
>
>   /// Returns the pair of line and column in the buffer.
>   /// If 'BufferID' is provided 'Loc' must come from that buffer.
>   std::pair<unsigned, unsigned>
>   getLineAndColumnInBuffer(SourceLoc Loc, unsigned BufferID = 0) const;
>
>   /// }
>
>   /// @{
>   /// Virtual buffer-name, line, and column.
>   /// These methods does respect '#sourceLocation'
>
>   /// Returns the virtual filename string for the Loc.
>   /// If 'BufferID' is provided 'Loc' must come from that buffer.
>   StringRef
>   getVirtualFilenameForLoc(SourceLoc Loc, unsigned BufferID = 0) const;
>
>   /// Returns the pair of line and column in the buffer
>   /// respecting #sourceLocation directive.
>   /// If 'BufferID' is provided 'Loc' must come from that buffer.
>   std::pair<unsigned, unsigned>
>   getVirtutalLineAndColumnForLoc(SourceLoc Loc, unsigned BufferID = 0)
> const;
>
>   /// }
>
> Roughly:
>
> Remove: getLineNumber
> ASIS: getIdentifierForBuffer(BufferID)
> Add: getIdentifierForBuffer(Loc) // overload
> Add:  getLineAndColumnInBuffer
> Rename: getBufferIdentifierForLoc => getVirtualFilenameForLoc
> Rename: getLineAndColumn => getVirtutalLineAndColumnForLoc
>
>
> *Audit every current usage of these methods*
>
> We already have several mix usage of them. For example:
>
> getLineNumber & getLineAndColumn
> https://github.com/apple/swift/blob/6293546/lib/AST/RawComment.cpp#L61-L63
>
> getIdentifierForBuffer & getLineAndColumn
> https://github.com/apple/swift/blob/6293546/lib/Frontend/
> SerializedDiagnosticConsumer.cpp#L242
> https://github.com/apple/swift/blob/6293546/lib/Frontend/
> SerializedDiagnosticConsumer.cpp#L451
>
> SourceFile::getFilename & getLineAndColumn
> https://github.com/apple/swift/blob/6293546/lib/SILGen/
> SILGenProfiling.cpp#L706
> https://github.com/apple/swift/blob/6293546/lib/SILGen/
> SILGenProfiling.cpp#L485-L486
>
> Also, I believe there are some places where we are using "virtutal" one
> even though we should use "real" one, and vise versa.
>
> I'm currently making a complete list of them. We should audit them all,
> and choose the correct methods. However, it's not clear for me that which
> one to use which methods; for example, I have no idea whether
> CoverageMapping intended to point the real locations or virtual.
>
>
> That's all I currently have.
> Could you please share your opinions on this before I proceed?
>
> --
> Rintaro
> _______________________________________________
> swift-dev mailing list
> swift-dev at swift.org
> https://lists.swift.org/mailman/listinfo/swift-dev
>
>
> _______________________________________________
> swift-dev mailing list
> swift-dev at swift.org
> https://lists.swift.org/mailman/listinfo/swift-dev
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.swift.org/pipermail/swift-dev/attachments/20170321/7a123e9e/attachment.html>