Add typo correction for the (non-template) type name in C++ "new" statements.

[Kaelyn, 2011-09-14 00:22:23]

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
>
>
>

[Kaelyn, 2011-09-22 22:01:11]

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
>>
>>
>>
>

[chandlerc, 2011-09-23 17:58:53]

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.

[dblaikie_gmail.com, 2011-09-23 18:12:39]

>
>
> 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

[Kaelyn, 2011-09-23 22:11:03]

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

[chandlerc, 2011-09-26 23:48:27]

FYI, looks fine to me pending a sanity check w/ Doug.