+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@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@swift.org <mailto:swift-dev@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@swift.org <mailto:swift-dev@swift.org>
>> https://lists.swift.org/mailman/listinfo/swift-dev
> 
> _______________________________________________
> swift-dev mailing list
> swift-dev@swift.org
> https://lists.swift.org/mailman/listinfo/swift-dev

_______________________________________________
swift-dev mailing list
swift-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-dev

Reply via email to