|
|
Patch Set 1 #
Total comments: 8
Patch Set 2 : Remove the need to pass a bool for controlling typo correction through several layers of the Parser #Patch Set 3 : Remove the need to pass a bool for controlling typo correction through several layers of the Parser #
Total messages: 8
|
Kaelyn
|
14 years, 4 months ago (2011年09月14日 00:16:25 UTC) #1 | |||||||||||||||||||||||||||||||||||||||
FYI, this patch allows for typo correction in what is from a user
perspective a reasonable case where clang should suggest a correction but
does not. Right now in:
namespace test {
class FizBinHelper {};
}
void foo() {
test::FizBinHelper *x;
x = new test::FizbinHelper;
}
clang will just say "expected a type" instead of suggesting correcting
test::FizbinHelper to test::FizBinHelper (fixing a simple case-mismatch
typo).
Just in case it wasn't clear what the context of the change is from the
changes to the test cases. ;)
Cheers,
Kaelyn
On Tue, Sep 13, 2011 at 5:16 PM, <rikka@google.com> wrote:
> Reviewers: cfe-commits_cs.uiuc.edu,
>
>
>
> Please review this at
http://codereview.appspot.com/**5014044/<http://codereview.appspot.com/5014044/>
>
> Affected files:
> M include/clang/Parse/Parser.h
> M include/clang/Sema/Sema.h
> M lib/Parse/ParseDecl.cpp
> M lib/Parse/ParseExprCXX.cpp
> M lib/Parse/Parser.cpp
> M lib/Sema/SemaDecl.cpp
> M test/SemaCXX/missing-**namespace-qualifier-typo-**corrections.cpp
>
>
>
Ping? I'd like at least a little feedback before submitting this patch since it is the first time I've done anything with the parser code. On Tue, Sep 13, 2011 at 5:22 PM, Kaelyn Uhrain <rikka@google.com> wrote: > FYI, this patch allows for typo correction in what is from a user > perspective a reasonable case where clang should suggest a correction but > does not. Right now in: > > namespace test { > class FizBinHelper {}; > } > > void foo() { > test::FizBinHelper *x; > x = new test::FizbinHelper; > } > > clang will just say "expected a type" instead of suggesting correcting > test::FizbinHelper to test::FizBinHelper (fixing a simple case-mismatch > typo). > > Just in case it wasn't clear what the context of the change is from the > changes to the test cases. ;) > > Cheers, > Kaelyn > > > On Tue, Sep 13, 2011 at 5:16 PM, <rikka@google.com> wrote: > >> Reviewers: cfe-commits_cs.uiuc.edu, >> >> >> >> Please review this at http://codereview.appspot.com/**5014044/<http://codereview.appspot.com/5014044/> >> >> Affected files: >> M include/clang/Parse/Parser.h >> M include/clang/Sema/Sema.h >> M lib/Parse/ParseDecl.cpp >> M lib/Parse/ParseExprCXX.cpp >> M lib/Parse/Parser.cpp >> M lib/Sema/SemaDecl.cpp >> M test/SemaCXX/missing-**namespace-qualifier-typo-**corrections.cpp >> >> >> >
I don't really like all the plumbing of bool parameters through so much of the parser. Why don't we *always* want typo correction when we call TryAnnotateTypeOrScopeToken? If there are good reasons, why can we not detect it from the immediate context? I looked at a few other calls, and didn't see any reason why we'd want to skip typo correction, but I didn't look at all of them. I think this really does need someone more deeply familiar with name lookup in the parser to look at it... http://codereview.appspot.com/5014044/diff/1/include/clang/Parse/Parser.h File include/clang/Parse/Parser.h (right): http://codereview.appspot.com/5014044/diff/1/include/clang/Parse/Parser.h#new... include/clang/Parse/Parser.h:1316: bool ParseCXXTypeSpecifierSeq(DeclSpec &DS, bool TryTypoCorrection=false); spaces around = http://codereview.appspot.com/5014044/diff/1/lib/Parse/ParseDecl.cpp File lib/Parse/ParseDecl.cpp (right): http://codereview.appspot.com/5014044/diff/1/lib/Parse/ParseDecl.cpp#newcode2241 lib/Parse/ParseDecl.cpp:2241: TemplateInfo, SuppressDeclarations, TryTypoCorrection); 80-columns http://codereview.appspot.com/5014044/diff/1/lib/Parse/ParseDecl.cpp#newcode2252 lib/Parse/ParseDecl.cpp:2252: TemplateInfo, SuppressDeclarations, TryTypoCorrection); 80-columns http://codereview.appspot.com/5014044/diff/1/lib/Parse/Parser.cpp File lib/Parse/Parser.cpp (right): http://codereview.appspot.com/5014044/diff/1/lib/Parse/Parser.cpp#newcode1282 lib/Parse/Parser.cpp:1282: IdentifierInfo *CorrectedII = NULL; s/NULL/0/ http://codereview.appspot.com/5014044/diff/1/lib/Parse/Parser.cpp#newcode1291 lib/Parse/Parser.cpp:1291: : NULL)) { s/NULL/0/ http://codereview.appspot.com/5014044/diff/1/lib/Sema/SemaDecl.cpp File lib/Sema/SemaDecl.cpp (right): http://codereview.appspot.com/5014044/diff/1/lib/Sema/SemaDecl.cpp#newcode150 lib/Sema/SemaDecl.cpp:150: TypoCorrection Corr = CorrectTypo(Result.getLookupNameInfo(), Can we use a name better than 'Corr'? http://codereview.appspot.com/5014044/diff/1/lib/Sema/SemaDecl.cpp#newcode165 lib/Sema/SemaDecl.cpp:165: std::string CorrectedStr(Corr.getAsString(getLangOptions())); Doesn't CorrecReplacement accept a StringRef? If so, can't we pass it a StringRef without creating an additional string? http://codereview.appspot.com/5014044/diff/1/lib/Sema/SemaDecl.cpp#newcode166 lib/Sema/SemaDecl.cpp:166: std::string CorrectedQuotedStr(Corr.getQuoted(getLangOptions())); Please stream the identifier info into the diagnostic. Formatting decisions such as this should be made entirely based on the type of the object being printed in the diagnostic and the diagnostic text in the table.
> > > http://codereview.appspot.com/5014044/diff/1/lib/Parse/Parser.cpp#newcode1282 > lib/Parse/Parser.cpp:1282: IdentifierInfo *CorrectedII = NULL; > s/NULL/0/ > I know it's fairly common, but certainly not universal, & I've been meaning to bring this up as a point of discussion (after the thread with Doug about potentially adding warnings for non-nullptr null pointer literals in C++0x): Should we consider NULL as the appropriate null pointer literal, rather than 0? Since it provides warning/checking that seems at least somewhat helpful. (& at some point I'd like to implement the style checker we discussed to verify that all null pointer literals are NULL, nullptr, etc) Otherwise if 0 is really considered the right way to go I'd be happy to go fix all the existing NULL usage, if that's preferred. - David
On Fri, Sep 23, 2011 at 10:58 AM, <chandlerc@gmail.com> wrote: > I don't really like all the plumbing of bool parameters through so much > of the parser. > > Why don't we *always* want typo correction when we call > TryAnnotateTypeOrScopeToken? If there are good reasons, why can we not > detect it from the immediate context? > > I looked at a few other calls, and didn't see any reason why we'd want > to skip typo correction, but I didn't look at all of them. I think this > really does need someone more deeply familiar with name lookup in the > parser to look at it... > 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. :) > > http://codereview.appspot.com/**5014044/diff/1/include/clang/** > Parse/Parser.h<http://codereview.appspot.com/5014044/diff/1/include/clang/Parse/Parser.h> > File include/clang/Parse/Parser.h (right): > > http://codereview.appspot.com/**5014044/diff/1/include/clang/** > Parse/Parser.h#newcode1316<http://codereview.appspot.com/5014044/diff/1/include/clang/Parse/Parser.h#newcode1316> > include/clang/Parse/Parser.h:**1316: bool > ParseCXXTypeSpecifierSeq(**DeclSpec &DS, bool TryTypoCorrection=false); > spaces around = > > http://codereview.appspot.com/**5014044/diff/1/lib/Parse/**ParseDecl.cpp<http... > File lib/Parse/ParseDecl.cpp (right): > > http://codereview.appspot.com/**5014044/diff/1/lib/Parse/** > ParseDecl.cpp#newcode2241<http://codereview.appspot.com/5014044/diff/1/lib/Parse/ParseDecl.cpp#newcode2241> > lib/Parse/ParseDecl.cpp:2241: TemplateInfo, SuppressDeclarations, > TryTypoCorrection); > 80-columns > > http://codereview.appspot.com/**5014044/diff/1/lib/Parse/** > ParseDecl.cpp#newcode2252<http://codereview.appspot.com/5014044/diff/1/lib/Parse/ParseDecl.cpp#newcode2252> > lib/Parse/ParseDecl.cpp:2252: TemplateInfo, SuppressDeclarations, > TryTypoCorrection); > 80-columns > The three above comments are now moot. ^.^ > > http://codereview.appspot.com/**5014044/diff/1/lib/Parse/**Parser.cpp<http://... > File lib/Parse/Parser.cpp (right): > > http://codereview.appspot.com/**5014044/diff/1/lib/Parse/** > Parser.cpp#newcode1282<http://codereview.appspot.com/5014044/diff/1/lib/Parse/Parser.cpp#newcode1282> > lib/Parse/Parser.cpp:1282: IdentifierInfo *CorrectedII = NULL; > s/NULL/0/ > > http://codereview.appspot.com/**5014044/diff/1/lib/Parse/** > Parser.cpp#newcode1291<http://codereview.appspot.com/5014044/diff/1/lib/Parse/Parser.cpp#newcode1291> > lib/Parse/Parser.cpp:1291: : NULL)) { > s/NULL/0/ > > http://codereview.appspot.com/**5014044/diff/1/lib/Sema/**SemaDecl.cpp<http:/... > File lib/Sema/SemaDecl.cpp (right): > > http://codereview.appspot.com/**5014044/diff/1/lib/Sema/** > SemaDecl.cpp#newcode150<http://codereview.appspot.com/5014044/diff/1/lib/Sema/SemaDecl.cpp#newcode150> > lib/Sema/SemaDecl.cpp:150: TypoCorrection Corr = > CorrectTypo(Result.**getLookupNameInfo(), > Can we use a name better than 'Corr'? > Missed this before uploading to rietveld. Changed to "Correction" and fixed the one line that went over 80 characters. > http://codereview.appspot.com/**5014044/diff/1/lib/Sema/** > SemaDecl.cpp#newcode165<http://codereview.appspot.com/5014044/diff/1/lib/Sema/SemaDecl.cpp#newcode165> > lib/Sema/SemaDecl.cpp:165: std::string > CorrectedStr(Corr.getAsString(**getLangOptions())); > Doesn't CorrecReplacement accept a StringRef? If so, can't we pass it a > StringRef without creating an additional string? > > http://codereview.appspot.com/**5014044/diff/1/lib/Sema/** > SemaDecl.cpp#newcode166<http://codereview.appspot.com/5014044/diff/1/lib/Sema/SemaDecl.cpp#newcode166> > lib/Sema/SemaDecl.cpp:166: std::string > CorrectedQuotedStr(Corr.**getQuoted(getLangOptions())); > Please stream the identifier info into the diagnostic. Formatting > decisions such as this should be made entirely based on the type of the > object being printed in the diagnostic and the diagnostic text in the > table. > 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. Cheers, Kaelyn