Would it be at all useful to you for it to return StringRef?<div><br></div><div>ZJ<br><br><div class="gmail_quote">On Tue, Jun 28, 2011 at 3:28 PM, jahanian <span dir="ltr"><<a href="mailto:fjahanian@apple.com">fjahanian@apple.com</a>></span> wrote:<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"><br><div><div class="im"><div>On Jun 28, 2011, at 11:43 AM, Zach Wheeler wrote:</div>
<br><blockquote type="cite">Below is a patch to kill Token::getName(). (Would it be better to attach patches rather than cut and paste them?)</blockquote><div><br></div></div>I frequently call it in the debugger to get the Token name. We should have something in its</div>
<div>place before killing it.</div><div><br></div><div>- Fariborz</div><div><div></div><div class="h5"><div><br><blockquote type="cite"><div><br></div><div>Clang built without complaint regarding this change.<br><div><div>
<br></div><div><br><div>
<br></div><div><div>Index: Token.h</div><div>===================================================================</div><div>--- Token.h<span style="white-space:pre-wrap"> </span>(revision 133884)</div><div>
+++ Token.h<span style="white-space:pre-wrap"> </span>(working copy)</div><div>@@ -145,10 +145,6 @@</div><div> setAnnotationEndLoc(R.getEnd());</div><div> }</div><div> </div><div>- const char *getName() const {</div>
<div>- return tok::getTokenName( (tok::TokenKind) Kind);</div><div>- }</div><div>-</div><div> /// startToken - Reset all flags to cleared.</div><div> ///</div><div> void startToken() {</div><div><br></div><br><div class="gmail_quote">
On Tue, Jun 28, 2011 at 12:03 AM, Chris Lattner <span dir="ltr"><<a href="mailto:clattner@apple.com" target="_blank">clattner@apple.com</a>></span> wrote:<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"><br><div><div><div>On Jun 27, 2011, at 7:56 PM, Zach Wheeler wrote:</div><br><blockquote type="cite">Token::getName() doesn't appear to be used presently, but it could (and perhaps should) be used on line <a href="http://clang.llvm.org/doxygen/classclang_1_1Preprocessor.html#ab6fec9812af95c3c0033ca180fc98ad0" target="_blank">159 of lib/Lex/Preprocessor.cpp</a> -- use "Tok.getName()" instead of "tok::getTokenName(Tok.getKind())".<br>
<br>Token::getName() just calls tok::getTokenName(); perhaps the former should return StringRef (since Token is an abstraction), and the latter should return const char* (since tok::getTokenName() is merely a procedure which returns a string constant)?<br>
<br>Maybe this particular issue doesn't matter enough to worry about all this though.<br></blockquote><div><br></div></div><div>Changing TokenKinds.h to return a stringref isn't compelling to me, but I'm not opposed to it. Changing the method in Token.h requires moving it out of line, adding the #include, or *removing it*. I vote for the later :)</div>
<div><br></div><font color="#888888"><div>-Chris</div></font><div><div></div><div><br><blockquote type="cite"><br>Thanks for your patience. :-)<br><br>ZJ<br><br><div class="gmail_quote">On Mon, Jun 27, 2011 at 8:14 PM, Chris Lattner <span dir="ltr"><<a href="mailto:clattner@apple.com" target="_blank">clattner@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin-top:0pt;margin-right:0pt;margin-bottom:0pt;margin-left:0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204, 204, 204);padding-left:1ex"><div style="word-wrap:break-word">
<br><div><div><div>On Jun 25, 2011, at 2:55 PM, Zach Wheeler wrote:</div>
<br><blockquote type="cite">This patch is intended to change <font face="'courier new', monospace">clang::Token::getName()</font> <font face="arial, helvetica, sans-serif"> so that it returns </font><font face="'courier new', monospace">llvm::StringRef</font><font face="arial, helvetica, sans-serif"> instead of </font><font face="'courier new', monospace">const char*</font><font face="arial, helvetica, sans-serif">.</font><div>
<div><font face="arial, helvetica, sans-serif"><br></font></div><div><font face="arial, helvetica, sans-serif">This is my first patch (ever), so if I did something I wasn't supposed to, just scream at me and I'll try to fix it. :-)</font></div>
</div><div><font face="arial, helvetica, sans-serif"><br></font></div><div><font face="arial, helvetica, sans-serif">This turned out to be an easy place to start; the doxygen reference indicates that this method isn't referenced at all. Clang built fine with these changes.</font></div>
</blockquote><div><br></div></div><div>Hi Zach,</div><div><br></div><div>This is a great start, but I'd prefer to not change these: if they aren't called at all, please send in a patch to nuke them :). Also, since these are just returning constant C strings, a "const char*" is actually good enough. The places that we'd like to convert to StringRef are ones that are manually implementing the same thing (by taking two const char*'s, or a pointer+size).</div>
<div><br></div><div>Thanks again for working on this area!</div><div><br></div><div>-Chris</div><br><blockquote type="cite"><div><div></div><div>
<div><font face="arial, helvetica, sans-serif"><br></font></div><div><font face="arial, helvetica, sans-serif">Regards, </font></div><div><font face="arial, helvetica, sans-serif">ZJ</font></div>
<div><font face="arial, helvetica, sans-serif"><br></font></div><div><font face="arial, helvetica, sans-serif"><br></font></div><div><font face="arial, helvetica, sans-serif"><br>
</font></div><div><font face="arial, helvetica, sans-serif"><div>Index: include/clang/Basic/TokenKinds.h</div><div>===================================================================</div><div>--- TokenKinds.h<span style="white-space:pre-wrap"> </span>(revision 133818)</div>
<div>+++ TokenKinds.h<span style="white-space:pre-wrap"> </span>(working copy)</div><div>@@ -14,6 +14,10 @@</div><div> #ifndef LLVM_CLANG_TOKENKINDS_H</div><div> #define LLVM_CLANG_TOKENKINDS_H</div><div>
</div><div>+namespace llvm {</div><div>+ class StringRef;</div><div>+}</div><div>+</div><div> namespace clang {</div><div> </div><div> namespace tok {</div><div>@@ -53,7 +57,7 @@</div><div> ///</div><div> /// The name of a token will be an internal name (such as "l_square")</div>
<div> /// and should not be used as part of diagnostic messages.</div><div>-const char *getTokenName(enum TokenKind Kind);</div><div>+llvm::StringRef getTokenName(enum TokenKind Kind);</div><div> </div><div> /// \brief Determines the spelling of simple punctuation tokens like</div>
<div> /// '!' or '%', and returns NULL for literal and annotation tokens.</div><div><br></div><div>Index: lib/Basic/TokenKinds.cpp</div><div>===================================================================</div>
<div>--- TokenKinds.cpp<span style="white-space:pre-wrap"> </span>(revision 133818)</div><div>+++ TokenKinds.cpp<span style="white-space:pre-wrap"> </span>(working copy)</div><div>@@ -12,7 +12,7 @@</div>
<div> //===----------------------------------------------------------------------===//</div><div> </div><div> #include "clang/Basic/TokenKinds.h"</div><div>-</div><div>+#include "llvm/ADT/StringRef.h"</div>
<div> #include <cassert></div><div> using namespace clang;</div><div> </div><div>@@ -23,9 +23,9 @@</div><div> 0</div><div> };</div><div> </div><div>-const char *tok::getTokenName(enum TokenKind Kind) {</div><div>
+llvm::StringRef tok::getTokenName(enum TokenKind Kind) {</div>
<div> assert(Kind < tok::NUM_TOKENS);</div><div>- return TokNames[Kind];</div><div>+ return llvm::StringRef(TokNames[Kind]);</div><div> }</div><div> </div><div> const char *tok::getTokenSimpleSpelling(enum TokenKind Kind) {</div>
<div><br></div><div>Index: include/clang/Lex/Token.h</div><div>===================================================================</div><div>--- Token.h<span style="white-space:pre-wrap"> </span>(revision 133818)</div>
<div>+++ Token.h<span style="white-space:pre-wrap"> </span>(working copy)</div><div>@@ -18,6 +18,7 @@</div><div> #include "clang/Basic/TokenKinds.h"</div><div> #include "clang/Basic/SourceLocation.h"</div>
<div> #include "clang/Basic/OperatorKinds.h"</div><div>+#include "llvm/ADT/StringRef.h"</div><div> #include <cstdlib></div><div> </div><div> namespace clang {</div><div>@@ -145,7 +146,7 @@</div>
<div>
setAnnotationEndLoc(R.getEnd());</div><div> }</div><div> </div><div>- const char *getName() const {</div><div>+ llvm::StringRef getName() const {</div><div> return tok::getTokenName( (tok::TokenKind) Kind);</div>
<div> }</div><div> </div><div><br></div></font></div></div></div>
_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div>
</blockquote></div><br><div style="display:inline"></div>
<div style="display:inline"></div>
</blockquote></div></div></div><br></div>
</blockquote></div><br></div></div></div></div>
_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div></div></div>
</blockquote></div><br></div>