<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class="">+1. The fact that APIs named “getLineNumber” and “getLineAndColumn” do subtly different things is pretty awful.</div><div class=""><br class=""></div><div class="">I like “presumed” better than “virtual”, but it’s not a big deal to me either way.</div><div class=""><br class=""></div><div class="">Ben</div><div class=""><br class=""></div><div class=""><br class=""></div><div><blockquote type="cite" class=""><div class="">On Mar 16, 2017, at 3:20 PM, Jordan Rose via swift-dev <<a href="mailto:swift-dev@swift.org" class="">swift-dev@swift.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><div class="">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.)</div><div class=""><br class=""></div><div class="">Jordan</div><div class=""><br class=""></div><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Mar 15, 2017, at 23:23, rintaro ishizaki via swift-dev <<a href="mailto:swift-dev@swift.org" class="">swift-dev@swift.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">Hi all,<div class=""><br class=""></div><div class="">I would like to propose to reorganize methods for retrieving filename, line and column from <font face="monospace, monospace" class="">SourceLoc</font>.</div><div class=""><br class=""></div><div class=""><b class=""><font size="4" class="">Motiavion</font></b></div><div class=""><br class=""></div><div class="">When we want to retrieve filename, line and column from <font face="monospace, monospace" class="">SourceLoc</font>, we have several ways to do that.</div><div class=""><br class=""></div><div class="">Filename:</div><div class=""><font face="monospace, monospace" class=""> SourceManager::getBufferIdentifierForLoc</font></div><div class=""><font face="monospace, monospace" class=""> SourceManager::getIdentifierForBuffer</font></div><div class=""><font face="monospace, monospace" class=""> SourceFile::getFilename<br class=""></font></div><div class=""><font face="monospace, monospace" class=""> LoadedFile::getFilename</font></div><div class=""><font face="monospace, monospace" class=""> llvm::MemoryBuffer::getBufferIdentifier</font></div><div class=""><br class=""></div><div class="">Line and Column:</div><div class=""><div class=""><font face="monospace, monospace" class=""> SourceManager::getLineAndColumn</font></div></div><div class=""><font face="monospace, monospace" class=""> SourceManager::getLineNumber</font></div><div class=""><br class=""></div><div class="">Some of them respect <font face="monospace, monospace" class="">#sourceLocation</font> directive, but some not:</div><div class=""><br class=""></div><div class="">Virtual (respect #sourceLocation):<br class=""><div class=""><font face="monospace, monospace" class=""> SourceManager::getBufferIdentifierForLoc</font></div></div><div class=""><div class=""><font face="monospace, monospace" class=""> SourceManager::getLineAndColumn</font></div></div><div class=""><font face="monospace, monospace" class=""><br class=""></font></div><div class="">Actual:</div><div class=""><span style="font-family:monospace,monospace" class=""> SourceManager::getIdentifierForBuffer</span><br class=""></div><div class=""><div class=""><font face="monospace, monospace" class=""> SourceFile::getFilename<br class=""></font></div><div class=""><font face="monospace, monospace" class=""> LoadedFile::getFilename</font></div><div class=""><font face="monospace, monospace" class=""> llvm::MemoryBuffer::getBufferIdentifier</font></div></div><div class=""><div class=""><font face="monospace, monospace" class=""> SourceManager::getLineNumber</font></div></div><br class="">Mixed use of them causes errors like <a href="https://github.com/apple/swift/pull/5425" class="">https://github.com/apple/swift/pull/5425</a><br class="">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* <font face="monospace, monospace" class="">getLineAndColumn</font>. When we want to retrieve the real line and column, we have use <font face="monospace, monospace" class="">getLineNumber(loc)</font> for the line, and <font face="monospace, monospace" class="">getLineAndColumn(loc).second</font> for the column. Most of our codebase doesn't bother to do that, though.<div class=""><br class=""><div class="">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.<br class=""></div><div class=""><br class=""></div><div class=""><br class=""></div><div class=""><font size="4" class=""><b class="">Proposed Solution</b></font></div><div class=""><br class=""></div><div class=""><b class="">Reorganize SourceManager methods.</b></div><div class=""><br class=""></div><div class="">We need methods for "real" one, and "virtual" one. And they need to have distinct name for that.</div><div class=""><br class=""></div><div class="">** RFC for method names **</div><div class=""><br class=""></div><div class=""><font face="monospace, monospace" class="">class SourceManager {</font></div><div class=""><br class=""></div><div class=""><font face="monospace, monospace" class="">public:</font></div><div class=""><font face="monospace, monospace" class=""><br class=""></font></div><div class=""><font face="monospace, monospace" color="#6aa84f" class=""> /// @{</font></div><div class=""><font face="monospace, monospace" color="#6aa84f" class=""> /// Real buffer-name, line, and column.</font></div><div class=""><font face="monospace, monospace" color="#6aa84f" class=""> /// These methods does not respect '#sourceLocation'</font></div><div class=""><font face="monospace, monospace" color="#6aa84f" class=""><br class=""></font></div><div class=""><font face="monospace, monospace" color="#6aa84f" class=""> /// Returns the identifier string for the buffer with the given ID.</font></div><div class=""><font face="monospace, monospace" color="#666666" class=""> StringRef</font></div><div class=""><font face="monospace, monospace" color="#666666" class=""> </font><font face="monospace, monospace" class="">getIdentifierForBuffer</font><font face="monospace, monospace" color="#666666" class="">(unsigned BufferID) const;<br class=""></font></div><div class=""><div class=""><font face="monospace, monospace" color="#666666" class=""><br class=""></font></div><div class=""><font face="monospace, monospace" color="#6aa84f" class=""> /// Returns the buffer identifier string for the Loc.</font></div><div class=""><font face="monospace, monospace" color="#6aa84f" class=""> /// If 'BufferID' is provided 'Loc' must come from that buffer.</font></div><div class=""><font face="monospace, monospace" color="#666666" class=""> StringRef</font></div><div class=""><font face="monospace, monospace" color="#666666" class=""> </font><span style="font-family: monospace, monospace;" class="">getIdentifierForBuffer</span><font face="monospace, monospace" color="#666666" class="">(SourceLoc Loc, unsigned BufferID = 0) const;</font></div></div><div class=""><font face="monospace, monospace" color="#666666" class=""><br class=""></font></div><div class=""><font face="monospace, monospace" color="#6aa84f" class=""> /// Returns the pair of line and column in the buffer.</font></div><div class=""><font face="monospace, monospace" color="#6aa84f" class=""> /// If 'BufferID' is provided 'Loc' must come from that buffer.</font></div><div class=""><font face="monospace, monospace" color="#666666" class=""> std::pair<unsigned, unsigned><br class=""></font></div><div class=""><font face="monospace, monospace" color="#666666" class=""> </font><font face="monospace, monospace" class="">getLineAndColumnInBuffer</font><font face="monospace, monospace" color="#666666" class="">(SourceLoc Loc, unsigned BufferID = 0) const;</font></div><div class=""><font face="monospace, monospace" color="#666666" class=""><br class=""></font></div><div class=""><font face="monospace, monospace" color="#6aa84f" class=""> /// }</font></div><div class=""><div class=""><font face="monospace, monospace" color="#6aa84f" class=""><br class=""></font></div><div class=""><font face="monospace, monospace" color="#6aa84f" class=""> /// @{</font></div><div class=""><font face="monospace, monospace" color="#6aa84f" class=""> /// Virtual buffer-name, line, and column.</font></div><div class=""><font face="monospace, monospace" color="#6aa84f" class=""> /// These methods does respect '#sourceLocation'</font></div><div class=""><font face="monospace, monospace" color="#6aa84f" class=""><br class=""></font></div><div class=""><div class=""><font face="monospace, monospace" color="#6aa84f" class=""> /// Returns the virtual filename string for the Loc.</font></div><div class=""><font face="monospace, monospace" color="#6aa84f" class=""> /// If 'BufferID' is provided 'Loc' must come from that buffer.</font></div><div class=""><font face="monospace, monospace" color="#666666" class=""> StringRef</font></div><div class=""><font face="monospace, monospace" color="#666666" class=""> </font><font face="monospace, monospace" class="">getVirtualFilenameForLoc</font><font face="monospace, monospace" color="#666666" class="">(SourceLoc Loc, unsigned BufferID = 0) const; </font></div></div><div class=""><font face="monospace, monospace" color="#666666" class=""><br class=""></font></div><div class=""><font face="monospace, monospace" color="#6aa84f" class=""> /// Returns the pair of line and column in the buffer</font></div><div class=""><font face="monospace, monospace" color="#6aa84f" class=""> /// respecting #sourceLocation directive.<br class=""></font></div><div class=""><font face="monospace, monospace" color="#6aa84f" class=""> /// If 'BufferID' is provided 'Loc' must come from that buffer.</font></div><div class=""><font face="monospace, monospace" color="#666666" class=""> std::pair<unsigned, unsigned><br class=""></font></div><div class=""><font face="monospace, monospace" color="#666666" class=""> </font><font face="monospace, monospace" class="">getVirtutalLineAndColumnForLoc</font><font face="monospace, monospace" color="#666666" class="">(SourceLoc Loc, unsigned BufferID = 0) const;</font></div><div class=""><font face="monospace, monospace" color="#666666" class=""><br class=""></font></div></div><div class=""><font face="monospace, monospace" color="#6aa84f" class=""> /// }</font></div><div class=""><br class=""></div><div class="">Roughly:</div><div class=""><div class=""><br class=""></div><div class="">Remove: <font face="monospace, monospace" class="">getLineNumber</font></div><div class=""></div><div class=""></div></div>ASIS: <font face="monospace, monospace" class="">getIdentifierForBuffer(BufferID)</font><div class="">Add: <span style="font-family:monospace,monospace" class="">getIdentifierForBuffer(Loc) // overload</span><br class="">Add: <font face="monospace, monospace" class="">getLineAndColumnInBuffer</font><br class="">Rename: <font face="monospace, monospace" class="">getBufferIdentifierForLoc</font> => <font face="monospace, monospace" class="">getVirtualFilenameForLoc</font><br class="">Rename: <font face="monospace, monospace" class="">getLineAndColumn</font> => <font face="monospace, monospace" class="">getVirtutalLineAndColumnForLoc</font></div><div class=""><div class=""><div class=""><br class=""></div><div class=""><br class=""></div><div class=""><div class=""><b class="">Audit every current usage of these methods</b></div></div></div></div><div class=""><b class=""><br class=""></b></div><div class="">We already have several mix usage of them. For example:</div><div class=""><br class=""></div><div class=""><font face="monospace, monospace" class="">getLineNumber & getLineAndColumn</font></div><div class=""><a href="https://github.com/apple/swift/blob/6293546/lib/AST/RawComment.cpp#L61-L63" class="">https://github.com/apple/swift/blob/6293546/lib/AST/RawComment.cpp#L61-L63</a><br class=""></div><div class=""><font face="monospace, monospace" class=""><br class=""></font></div><div class=""><font face="monospace, monospace" class="">getIdentifierForBuffer & getLineAndColumn</font></div><div class=""><a href="https://github.com/apple/swift/blob/6293546/lib/Frontend/SerializedDiagnosticConsumer.cpp#L242" class="">https://github.com/apple/swift/blob/6293546/lib/Frontend/SerializedDiagnosticConsumer.cpp#L242</a><br class=""></div><div class=""><a href="https://github.com/apple/swift/blob/6293546/lib/Frontend/SerializedDiagnosticConsumer.cpp#L451" class="">https://github.com/apple/swift/blob/6293546/lib/Frontend/SerializedDiagnosticConsumer.cpp#L451</a><br class=""></div><div class=""><br class=""></div><div class="">SourceFile::getFilename & <span style="font-family:monospace,monospace" class="">getLineAndColumn</span></div><div class=""><font face="monospace, monospace" class=""><a href="https://github.com/apple/swift/blob/6293546/lib/SILGen/SILGenProfiling.cpp#L706" class="">https://github.com/apple/swift/blob/6293546/lib/SILGen/SILGenProfiling.cpp#L706</a></font><br class=""></div><div class=""><font face="monospace, monospace" class=""><a href="https://github.com/apple/swift/blob/6293546/lib/SILGen/SILGenProfiling.cpp#L485-L486" class="">https://github.com/apple/swift/blob/6293546/lib/SILGen/SILGenProfiling.cpp#L485-L486</a><br class=""></font></div><div class=""><br class=""></div><div class="">Also, I believe there are some places where we are using "virtutal" one even though we should use "real" one, and vise versa.</div><div class=""><br class=""></div><div class="">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 <font face="monospace, monospace" class="">CoverageMapping</font> intended to point the real locations or virtual.</div><div class=""><br class=""></div><div class=""><br class=""></div><div class="">That's all I currently have.</div><div class="">Could you please share your opinions on this before I proceed?</div><div class=""><br class=""></div></div><div class="">--</div><div class="">Rintaro</div></div>
_______________________________________________<br class="">swift-dev mailing list<br class=""><a href="mailto:swift-dev@swift.org" class="">swift-dev@swift.org</a><br class=""><a href="https://lists.swift.org/mailman/listinfo/swift-dev" class="">https://lists.swift.org/mailman/listinfo/swift-dev</a><br class=""></div></blockquote></div><br class=""></div>_______________________________________________<br class="">swift-dev mailing list<br class=""><a href="mailto:swift-dev@swift.org" class="">swift-dev@swift.org</a><br class="">https://lists.swift.org/mailman/listinfo/swift-dev<br class=""></div></blockquote></div><br class=""></body></html>