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

[rintaro ishizaki]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.swift.org/pipermail/swift-dev/attachments/20170316/55b3b088/attachment.html>