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

[Ben Langmuir]

+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 <mailto: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 <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 <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#L242>
>> https://github.com/apple/swift/blob/6293546/lib/Frontend/SerializedDiagnosticConsumer.cpp#L451 <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#L706>
>> https://github.com/apple/swift/blob/6293546/lib/SILGen/SILGenProfiling.cpp#L485-L486 <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 <mailto: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/20170316/b3d21151/attachment.html>