<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Jun 27, 2011, at 7:56 PM, Zach Wheeler wrote:</div><br class="Apple-interchange-newline"><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>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><div>-Chris</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: 0pt 0pt 0pt 0.8ex; border-left: 1px solid 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="visibility: hidden; display: inline;" id="avg_ls_inline_popup"></div><style type="text/css">#avg_ls_inline_popup { position:absolute; z-index:9999; padding: 0px 0px; margin-left: 0px; margin-top: 0px; width: 240px; overflow: hidden; word-wrap: break-word; color: black; font-size: 10px; text-align: left; line-height: 13px;}</style>
</blockquote></div><br></body></html>