<div class="gmail_quote">On Fri, Sep 23, 2011 at 10:58 AM,  <span dir="ltr"><<a href="mailto:chandlerc@gmail.com">chandlerc@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
I don't really like all the plumbing of bool parameters through so much<br>
of the parser.<br>
<br>
Why don't we *always* want typo correction when we call<br>
TryAnnotateTypeOrScopeToken? If there are good reasons, why can we not<br>
detect it from the immediate context?<br>
<br>
I looked at a few other calls, and didn't see any reason why we'd want<br>
to skip typo correction, but I didn't look at all of them. I think this<br>
really does need someone more deeply familiar with name lookup in the<br>
parser to look at it...<br></blockquote><div><br>Thanks for prompting me to reexamine the patch with fresh eyes! I was able to fix the problems in the typo correction code  in Sema::getTypeName and to remove the bool parameters I'd added to the parser. :)<br>
<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
<br>
<a href="http://codereview.appspot.com/5014044/diff/1/include/clang/Parse/Parser.h" target="_blank">http://codereview.appspot.com/<u></u>5014044/diff/1/include/clang/<u></u>Parse/Parser.h</a><br>
File include/clang/Parse/Parser.h (right):<br>
<br>
<a href="http://codereview.appspot.com/5014044/diff/1/include/clang/Parse/Parser.h#newcode1316" target="_blank">http://codereview.appspot.com/<u></u>5014044/diff/1/include/clang/<u></u>Parse/Parser.h#newcode1316</a><br>
include/clang/Parse/Parser.h:<u></u>1316: bool<br>
ParseCXXTypeSpecifierSeq(<u></u>DeclSpec &DS, bool TryTypoCorrection=false);<br>
spaces around =<br>
<br>
<a href="http://codereview.appspot.com/5014044/diff/1/lib/Parse/ParseDecl.cpp" target="_blank">http://codereview.appspot.com/<u></u>5014044/diff/1/lib/Parse/<u></u>ParseDecl.cpp</a><br>
File lib/Parse/ParseDecl.cpp (right):<br>
<br>
<a href="http://codereview.appspot.com/5014044/diff/1/lib/Parse/ParseDecl.cpp#newcode2241" target="_blank">http://codereview.appspot.com/<u></u>5014044/diff/1/lib/Parse/<u></u>ParseDecl.cpp#newcode2241</a><br>
lib/Parse/ParseDecl.cpp:2241: TemplateInfo, SuppressDeclarations,<br>
TryTypoCorrection);<br>
80-columns<br>
<br>
<a href="http://codereview.appspot.com/5014044/diff/1/lib/Parse/ParseDecl.cpp#newcode2252" target="_blank">http://codereview.appspot.com/<u></u>5014044/diff/1/lib/Parse/<u></u>ParseDecl.cpp#newcode2252</a><br>
lib/Parse/ParseDecl.cpp:2252: TemplateInfo, SuppressDeclarations,<br>
TryTypoCorrection);<br>
80-columns<br></blockquote><div><br>The three above comments are now moot. ^.^<br> <br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">

<br>
<a href="http://codereview.appspot.com/5014044/diff/1/lib/Parse/Parser.cpp" target="_blank">http://codereview.appspot.com/<u></u>5014044/diff/1/lib/Parse/<u></u>Parser.cpp</a><br>
File lib/Parse/Parser.cpp (right):<br>
<br>
<a href="http://codereview.appspot.com/5014044/diff/1/lib/Parse/Parser.cpp#newcode1282" target="_blank">http://codereview.appspot.com/<u></u>5014044/diff/1/lib/Parse/<u></u>Parser.cpp#newcode1282</a><br>
lib/Parse/Parser.cpp:1282: IdentifierInfo *CorrectedII = NULL;<br>
s/NULL/0/<br>
<br>
<a href="http://codereview.appspot.com/5014044/diff/1/lib/Parse/Parser.cpp#newcode1291" target="_blank">http://codereview.appspot.com/<u></u>5014044/diff/1/lib/Parse/<u></u>Parser.cpp#newcode1291</a><br>
lib/Parse/Parser.cpp:1291: : NULL)) {<br>
s/NULL/0/<br>
<br>
<a href="http://codereview.appspot.com/5014044/diff/1/lib/Sema/SemaDecl.cpp" target="_blank">http://codereview.appspot.com/<u></u>5014044/diff/1/lib/Sema/<u></u>SemaDecl.cpp</a><br>
File lib/Sema/SemaDecl.cpp (right):<br>
<br>
<a href="http://codereview.appspot.com/5014044/diff/1/lib/Sema/SemaDecl.cpp#newcode150" target="_blank">http://codereview.appspot.com/<u></u>5014044/diff/1/lib/Sema/<u></u>SemaDecl.cpp#newcode150</a><br>
lib/Sema/SemaDecl.cpp:150: TypoCorrection Corr =<br>
CorrectTypo(Result.<u></u>getLookupNameInfo(),<br>
Can we use a name better than 'Corr'?<br></blockquote><div><br>Missed this before uploading to rietveld. Changed to "Correction" and fixed the one line that went over 80 characters.<br><br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">

<br>
<a href="http://codereview.appspot.com/5014044/diff/1/lib/Sema/SemaDecl.cpp#newcode165" target="_blank">http://codereview.appspot.com/<u></u>5014044/diff/1/lib/Sema/<u></u>SemaDecl.cpp#newcode165</a><br>
lib/Sema/SemaDecl.cpp:165: std::string<br>
CorrectedStr(Corr.getAsString(<u></u>getLangOptions()));<br>
Doesn't CorrecReplacement accept a StringRef? If so, can't we pass it a<br>
StringRef without creating an additional string?<br>
<br>
<a href="http://codereview.appspot.com/5014044/diff/1/lib/Sema/SemaDecl.cpp#newcode166" target="_blank">http://codereview.appspot.com/<u></u>5014044/diff/1/lib/Sema/<u></u>SemaDecl.cpp#newcode166</a><br>
lib/Sema/SemaDecl.cpp:166: std::string<br>
CorrectedQuotedStr(Corr.<u></u>getQuoted(getLangOptions()));<br>
Please stream the identifier info into the diagnostic. Formatting<br>
decisions such as this should be made entirely based on the type of the<br>
object being printed in the diagnostic and the diagnostic text in the<br>
table.<br></blockquote><div><br>The reason for calling getAsString() and getQuoted() is because the typo correction may include a namespace qualifier, and those functions build a string containing the minimally qualified string needed to reference the new identifier in the appropriate namespace. See test/SemaCXX/msising-namespace-qualifier-typo-corrections.cpp for lots of examples where the displayed string has to be build up from a new NestedNamespaceQualifier and DeclarationName/IdentifierInfo but for which e.g. calling the associated NamedDecl's getQualifiedNameAsString() method returns the wrong thing.<br>
<br>Cheers,<br>Kaelyn<br></div></div><br>