<div dir="ltr">Thanks!<div><br></div><div><div>I'd go with "presumed".</div></div><div><br></div><div>I've just started a WIP pull request.</div><div><a href="https://github.com/apple/swift/pull/8203">https://github.com/apple/swift/pull/8203</a><br></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">2017-03-17 7:37 GMT+09:00 Ben Langmuir <span dir="ltr"><<a href="mailto:blangmuir@apple.com" target="_blank">blangmuir@apple.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>+1. The fact that APIs named “getLineNumber” and “getLineAndColumn” do subtly different things is pretty awful.</div><div><br></div><div>I like “presumed” better than “virtual”, but it’s not a big deal to me either way.</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>Ben</div></font></span><div><div class="h5"><div><br></div><div><br></div><div><blockquote type="cite"><div>On Mar 16, 2017, at 3:20 PM, Jordan Rose via swift-dev <<a href="mailto:swift-dev@swift.org" target="_blank">swift-dev@swift.org</a>> wrote:</div><br class="m_-3349925255050073896Apple-interchange-newline"><div><div style="word-wrap:break-word;line-break:after-white-space"><div>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><br></div><div>Jordan</div><div><br></div><br><div><blockquote type="cite"><div>On Mar 15, 2017, at 23:23, rintaro ishizaki via swift-dev <<a href="mailto:swift-dev@swift.org" target="_blank">swift-dev@swift.org</a>> wrote:</div><br class="m_-3349925255050073896Apple-interchange-newline"><div><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::<wbr>getBufferIdentifierForLoc</font></div><div><font face="monospace, monospace"> SourceManager::<wbr>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::<wbr>getBufferIdentifier</font></div><div><br></div><div>Line and Column:</div><div><div><font face="monospace, monospace"> SourceManager::<wbr>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::<wbr>getBufferIdentifierForLoc</font></div></div><div><div><font face="monospace, monospace"> SourceManager::<wbr>getLineAndColumn</font></div></div><div><font face="monospace, monospace"><br></font></div><div>Actual:</div><div><span style="font-family:monospace,monospace"> SourceManager::<wbr>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::<wbr>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" target="_blank">https://github.com/apple/<wbr>swift/pull/5425</a><br>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">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'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 "real" 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 "real" one, and "virtual" 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 '#sourceLocation'</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">getIdentifierForBuffer</font><font face="monospace, monospace" color="#666666">(<wbr>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 'BufferID' is provided 'Loc' 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="font-family:monospace,monospace">getIdentifierForBuffer</span><font face="monospace, monospace" color="#666666">(<wbr>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 'BufferID' is provided 'Loc' must come from that buffer.</font></div><div><font face="monospace, monospace" color="#666666"> std::pair<unsigned, unsigned><br></font></div><div><font face="monospace, monospace" color="#666666"> </font><font face="monospace, monospace">getLineAndColumnInBuffer</font><font face="monospace, monospace" color="#666666">(<wbr>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 '#sourceLocation'</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 'BufferID' is provided 'Loc' 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">getVirtualFilenameForLoc</font><font face="monospace, monospace" color="#666666">(<wbr>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 'BufferID' is provided 'Loc' must come from that buffer.</font></div><div><font face="monospace, monospace" color="#666666"> std::pair<unsigned, unsigned><br></font></div><div><font face="monospace, monospace" color="#666666"> </font><font face="monospace, monospace">getVirtutalLineAndColumnForLoc</font><font face="monospace, monospace" color="#666666"><wbr>(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(<wbr>BufferID)</font><div>Add: <span style="font-family:monospace,monospace">getIdentifierForBuffer(<wbr>Loc) // overload</span><br>Add: <font face="monospace, monospace">getLineAndColumnInBuffer</font><br>Rename: <font face="monospace, monospace">getBufferIdentifierForLoc</font> => <font face="monospace, monospace">getVirtualFilenameForLoc</font><br>Rename: <font face="monospace, monospace">getLineAndColumn</font> => <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 & getLineAndColumn</font></div><div><a href="https://github.com/apple/swift/blob/6293546/lib/AST/RawComment.cpp#L61-L63" target="_blank">https://github.com/apple/<wbr>swift/blob/6293546/lib/AST/<wbr>RawComment.cpp#L61-L63</a><br></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">getIdentifierForBuffer & getLineAndColumn</font></div><div><a href="https://github.com/apple/swift/blob/6293546/lib/Frontend/SerializedDiagnosticConsumer.cpp#L242" target="_blank">https://github.com/apple/<wbr>swift/blob/6293546/lib/<wbr>Frontend/<wbr>SerializedDiagnosticConsumer.<wbr>cpp#L242</a><br></div><div><a href="https://github.com/apple/swift/blob/6293546/lib/Frontend/SerializedDiagnosticConsumer.cpp#L451" target="_blank">https://github.com/apple/<wbr>swift/blob/6293546/lib/<wbr>Frontend/<wbr>SerializedDiagnosticConsumer.<wbr>cpp#L451</a><br></div><div><br></div><div>SourceFile::getFilename & <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" target="_blank">https://github.com/apple/<wbr>swift/blob/6293546/lib/SILGen/<wbr>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" target="_blank">https://github.com/apple/<wbr>swift/blob/6293546/lib/SILGen/<wbr>SILGenProfiling.cpp#L485-L486</a><br></font></div><div><br></div><div>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><br></div><div>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">CoverageMapping</font> intended to point the real locations or virtual.</div><div><br></div><div><br></div><div>That'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>
______________________________<wbr>_________________<br>swift-dev mailing list<br><a href="mailto:swift-dev@swift.org" target="_blank">swift-dev@swift.org</a><br><a href="https://lists.swift.org/mailman/listinfo/swift-dev" target="_blank">https://lists.swift.org/<wbr>mailman/listinfo/swift-dev</a><br></div></blockquote></div><br></div>______________________________<wbr>_________________<br>swift-dev mailing list<br><a href="mailto:swift-dev@swift.org" target="_blank">swift-dev@swift.org</a><br><a href="https://lists.swift.org/mailman/listinfo/swift-dev" target="_blank">https://lists.swift.org/<wbr>mailman/listinfo/swift-dev</a><br></div></blockquote></div><br></div></div></div></blockquote></div><br></div>