Filename, Line and Column for SourceLoc


#1

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


(Jordan Rose) #2

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> 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@swift.org
https://lists.swift.org/mailman/listinfo/swift-dev


(Ben Langmuir) #3

+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
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@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


#4

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@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@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> 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@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