<div dir="ltr">Hi all,<div><br></div><div>I would like to propose to reorganize methods for retrieving filename, line and column from <font face="monospace, monospace">SourceLoc</font>.</div><div><br></div><div><b><font size="4">Motiavion</font></b></div><div><br></div><div>When we want to retrieve filename, line and column from <font face="monospace, monospace">SourceLoc</font>, we have several ways to do that.</div><div><br></div><div>Filename:</div><div><font face="monospace, monospace">  SourceManager::getBufferIdentifierForLoc</font></div><div><font face="monospace, monospace">  SourceManager::getIdentifierForBuffer</font></div><div><font face="monospace, monospace">  SourceFile::getFilename<br></font></div><div><font face="monospace, monospace">  LoadedFile::getFilename</font></div><div><font face="monospace, monospace">  llvm::MemoryBuffer::getBufferIdentifier</font></div><div><br></div><div>Line and Column:</div><div><div><font face="monospace, monospace">  SourceManager::getLineAndColumn</font></div></div><div><font face="monospace, monospace">  SourceManager::getLineNumber</font></div><div><br></div><div>Some of them respect <font face="monospace, monospace">#sourceLocation</font> directive, but some not:</div><div><br></div><div>Virtual (respect #sourceLocation):<br><div><font face="monospace, monospace">  SourceManager::getBufferIdentifierForLoc</font></div></div><div><div><font face="monospace, monospace">  SourceManager::getLineAndColumn</font></div></div><div><font face="monospace, monospace"><br></font></div><div>Actual:</div><div><span style="font-family:monospace,monospace">  SourceManager::getIdentifierForBuffer</span><br></div><div><div><font face="monospace, monospace">  SourceFile::getFilename<br></font></div><div><font face="monospace, monospace">  LoadedFile::getFilename</font></div><div><font face="monospace, monospace">  llvm::MemoryBuffer::getBufferIdentifier</font></div></div><div><div><font face="monospace, monospace">  SourceManager::getLineNumber</font></div></div><br>Mixed use of them causes errors like <a href="https://github.com/apple/swift/pull/5425">https://github.com/apple/swift/pull/5425</a><br>Since current method namings are unclear about this, I think it&#39;s very error prone. Also, we currently don&#39;t have any method for *real* <font face="monospace, monospace">getLineAndColumn</font>. When we want to retrieve the real line and column, we have use <font face="monospace, monospace">getLineNumber(loc)</font> for the line, and <font face="monospace, monospace">getLineAndColumn(loc).second</font> for the column. Most of our codebase doesn&#39;t bother to do that, though.<div><br><div>I suspect that Xcode crash when using #sourceLocation in PlayGround is probably caused by this kind of problem; i.e. Xcode expects &quot;real&quot; line and column, but the compiler or sourcekit returns virtual one.<br></div><div><br></div><div><br></div><div><font size="4"><b>Proposed Solution</b></font></div><div><br></div><div><b>Reorganize SourceManager methods.</b></div><div><br></div><div>We need methods for &quot;real&quot; one, and &quot;virtual&quot; one. And they need to have distinct name for that.</div><div><br></div><div>** RFC for method names **</div><div><br></div><div><font face="monospace, monospace">class SourceManager {</font></div><div><br></div><div><font face="monospace, monospace">public:</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace" color="#6aa84f">  /// @{</font></div><div><font face="monospace, monospace" color="#6aa84f">  /// Real buffer-name, line, and column.</font></div><div><font face="monospace, monospace" color="#6aa84f">  /// These methods does not respect &#39;#sourceLocation&#39;</font></div><div><font face="monospace, monospace" color="#6aa84f"><br></font></div><div><font face="monospace, monospace" color="#6aa84f">  /// Returns the identifier string for the buffer with the given ID.</font></div><div><font face="monospace, monospace" color="#666666">  StringRef</font></div><div><font face="monospace, monospace" color="#666666">  </font><font face="monospace, monospace" color="#000000">getIdentifierForBuffer</font><font face="monospace, monospace" color="#666666">(unsigned BufferID) const;<br></font></div><div><div><font face="monospace, monospace" color="#666666"><br></font></div><div><font face="monospace, monospace" color="#6aa84f">  /// Returns the buffer identifier string for the Loc.</font></div><div><font face="monospace, monospace" color="#6aa84f">  /// If &#39;BufferID&#39; is provided &#39;Loc&#39; must come from that buffer.</font></div><div><font face="monospace, monospace" color="#666666">  StringRef</font></div><div><font face="monospace, monospace" color="#666666">  </font><span style="color:rgb(0,0,0);font-family:monospace,monospace">getIdentifierForBuffer</span><font face="monospace, monospace" color="#666666">(SourceLoc Loc, unsigned BufferID = 0) const;</font></div></div><div><font face="monospace, monospace" color="#666666"><br></font></div><div><font face="monospace, monospace" color="#6aa84f">  /// Returns the pair of line and column in the buffer.</font></div><div><font face="monospace, monospace" color="#6aa84f">  /// If &#39;BufferID&#39; is provided &#39;Loc&#39; must come from that buffer.</font></div><div><font face="monospace, monospace" color="#666666">  std::pair&lt;unsigned, unsigned&gt;<br></font></div><div><font face="monospace, monospace" color="#666666">  </font><font face="monospace, monospace" color="#000000">getLineAndColumnInBuffer</font><font face="monospace, monospace" color="#666666">(SourceLoc Loc, unsigned BufferID = 0) const;</font></div><div><font face="monospace, monospace" color="#666666"><br></font></div><div><font face="monospace, monospace" color="#6aa84f">  /// }</font></div><div><div><font face="monospace, monospace" color="#6aa84f"><br></font></div><div><font face="monospace, monospace" color="#6aa84f">  /// @{</font></div><div><font face="monospace, monospace" color="#6aa84f">  /// Virtual buffer-name, line, and column.</font></div><div><font face="monospace, monospace" color="#6aa84f">  /// These methods does respect &#39;#sourceLocation&#39;</font></div><div><font face="monospace, monospace" color="#6aa84f"><br></font></div><div><div><font face="monospace, monospace" color="#6aa84f">  /// Returns the virtual filename string for the Loc.</font></div><div><font face="monospace, monospace" color="#6aa84f">  /// If &#39;BufferID&#39; is provided &#39;Loc&#39; must come from that buffer.</font></div><div><font face="monospace, monospace" color="#666666">  StringRef</font></div><div><font face="monospace, monospace" color="#666666">  </font><font face="monospace, monospace" color="#000000">getVirtualFilenameForLoc</font><font face="monospace, monospace" color="#666666">(SourceLoc Loc, unsigned BufferID = 0) const;  </font></div></div><div><font face="monospace, monospace" color="#666666"><br></font></div><div><font face="monospace, monospace" color="#6aa84f">  /// Returns the pair of line and column in the buffer</font></div><div><font face="monospace, monospace" color="#6aa84f">  /// respecting #sourceLocation directive.<br></font></div><div><font face="monospace, monospace" color="#6aa84f">  /// If &#39;BufferID&#39; is provided &#39;Loc&#39; must come from that buffer.</font></div><div><font face="monospace, monospace" color="#666666">  std::pair&lt;unsigned, unsigned&gt;<br></font></div><div><font face="monospace, monospace" color="#666666">  </font><font face="monospace, monospace" color="#000000">getVirtutalLineAndColumnForLoc</font><font face="monospace, monospace" color="#666666">(SourceLoc Loc, unsigned BufferID = 0) const;</font></div><div><font face="monospace, monospace" color="#666666"><br></font></div></div><div><font face="monospace, monospace" color="#6aa84f">  /// }</font></div><div><br></div><div>Roughly:</div><div><div><br></div><div>Remove: <font face="monospace, monospace">getLineNumber</font></div><div></div><div></div></div>ASIS: <font face="monospace, monospace">getIdentifierForBuffer(BufferID)</font><div>Add: <span style="font-family:monospace,monospace">getIdentifierForBuffer(Loc) // overload</span><br>Add:  <font face="monospace, monospace">getLineAndColumnInBuffer</font><br>Rename: <font face="monospace, monospace">getBufferIdentifierForLoc</font> =&gt; <font face="monospace, monospace">getVirtualFilenameForLoc</font><br>Rename: <font face="monospace, monospace">getLineAndColumn</font> =&gt; <font face="monospace, monospace">getVirtutalLineAndColumnForLoc</font></div><div><div><div><br></div><div><br></div><div><div><b>Audit every current usage of these methods</b></div></div></div></div><div><b><br></b></div><div>We already have several mix usage of them. For example:</div><div><br></div><div><font face="monospace, monospace">getLineNumber &amp; getLineAndColumn</font></div><div><a href="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</a><br></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">getIdentifierForBuffer &amp; getLineAndColumn</font></div><div><a href="https://github.com/apple/swift/blob/6293546/lib/Frontend/SerializedDiagnosticConsumer.cpp#L242">https://github.com/apple/swift/blob/6293546/lib/Frontend/SerializedDiagnosticConsumer.cpp#L242</a><br></div><div><a href="https://github.com/apple/swift/blob/6293546/lib/Frontend/SerializedDiagnosticConsumer.cpp#L451">https://github.com/apple/swift/blob/6293546/lib/Frontend/SerializedDiagnosticConsumer.cpp#L451</a><br></div><div><br></div><div>SourceFile::getFilename &amp; <span style="font-family:monospace,monospace">getLineAndColumn</span></div><div><font face="monospace, monospace"><a href="https://github.com/apple/swift/blob/6293546/lib/SILGen/SILGenProfiling.cpp#L706">https://github.com/apple/swift/blob/6293546/lib/SILGen/SILGenProfiling.cpp#L706</a></font><br></div><div><font face="monospace, monospace"><a href="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</a><br></font></div><div><br></div><div>Also, I believe there are some places where we are using &quot;virtutal&quot; one even though we should use &quot;real&quot; one, and vise versa.</div><div><br></div><div>I&#39;m currently making a complete list of them. We should audit them all, and choose the correct methods. However, it&#39;s not clear for me that which one to use which methods; for example, I have no idea whether <font face="monospace, monospace">CoverageMapping</font> intended to point the real locations or virtual.</div><div><br></div><div><br></div><div>That&#39;s all I currently have.</div><div>Could you please share your opinions on this before I proceed?</div><div><br></div></div><div>--</div><div>Rintaro</div></div>