[cfe-commits] PATCH: Add support for C++ namespace-aware typo correcting

[Douglas Gregor]

On Jun 27, 2011, at 5:07 PM, Kaelyn Uhrain wrote:

> And here is the second half, which changes the rest of the callers of CorrectTypo, expands the unit test, adds a new test case to test/FixIt/typo.cpp to verify the namespace qualifier correction is applied properly, and removes the CorrectTypo compatibility wrapper. It should be applied on top of the previous patch (ns-pt1-20110627.diff).
> 
> Both patches are based on the tip of http://llvm.org/git/clang as of ~3pm PDT today. All unit tests pass both with only the first patch applied and with this patch applied.

I've gone ahead and committed this as r134007, with a few minor tweaks + PCH support. Thanks so much for working on this!

	- Doug

> Cheers,
> Kaelyn
> 
> On Mon, Jun 27, 2011 at 5:01 PM, Kaelyn Uhrain <rikka at google.com> wrote:
> So in hopes of having my changes finally committed soon, I've modified basic.lookup.argdep/p4.cpp to expect the messages triggered by trying to correct func(B::B()) to use C::func and failing on the conversions from class B::B to C::C, along with a lengthy FIXME comment. Playing around with the code a bit, there doesn't seem to be a good way to resolve this in the clang code base as it is, since it would effectively require--in some manner--checking the "corrected" function can work with the given arguments before emitting any diagnostic messages about correcting the function name or about attempted argument type conversions (including the lack of viable copy-type constructors). I've also removed the requiresADL boolean from CorrectTypo since it is no longer needed and was incorrect to begin with since it only noted that an expression was a candidate for ADL, not that it was determined ADL was definitely necessary.
> 
> Attached is the first patch with the changes to CorrectTypo and to one caller of CorrectTypo (Sema::DiagnoseEmptyLookup in SemaExpr.cpp), along with a basic unit test that exercises the new CorrectTypo behavior via DiagnoseEmptyLookup.
> 
> 
> On Fri, Jun 24, 2011 at 1:44 PM, Kaelyn Uhrain <rikka at google.com> wrote:
> FYI, I have all of the CorrectTypo call sites updated and everything working (including fixit mode, which didn't require mucking with the CXXScopeSpec [and said mucking screws up diagnostic messages]); I'm just waiting on guidance for resolving the issues with basic.lookup.argdep/p4.cpp. I'm tempted to just fix up the expectations in basic.lookup.argdep/p4.cpp, except the old error message is far more useful than the new and misleading/bogus messages and I'd like to figure out a way to keep the old behavior if possible without severely crippling the missing-namespace corrections.
> 
> Cheers,
> Kaelyn
> 
> 
> On Thu, Jun 23, 2011 at 5:11 PM, Kaelyn Uhrain <rikka at google.com> wrote:
> +    // FIXME: Don't do the lookups if argument dependent lookup is required as
> +    // this will break test/CXX/basic/basic.lookup/basic.lookup.argdep/p4.cpp
> 
> Is this FIXME still relevant, given that we're doing the appropriate requiresADL check?
> 
> I've found a problem with the simple ADL check in CorrectTypo... with it, common cases we want to suggest corrections for such as:
> 
> namespace fizbin {
>   namespace nested { bool moreFoobar() { return true; } }
> }
> void foo() {
>   moreFoobar();
> }
> 
> give:
> 
> tmp.cpp:5:3: error: use of undeclared identifier 'moreFoobar'
>   moreFoobar();
>   ^
> 1 error generated.
> 
> instead of:
> 
> tmp.cpp:5:3: error: use of undeclared identifier 'moreFoobar'; did you mean
>       'fizbin::nested::moreFoobar'?
>   moreFoobar();
>   ^~~~~~~~~~
>   fizbin::nested::moreFoobar
> tmp.cpp:2:27: note: 'fizbin::nested::moreFoobar' declared here
>   namespace nested { bool moreFoobar() { return true; } }
> 
> but without the check, basic.lookup.argdep/p4.cpp reports:
> 
> error: 'error' diagnostics seen but not expected: 
>   Line 35: no viable conversion from 'B::B' to 'C::C'
> 
> error: 'note' diagnostics seen but not expected: 
>   Line 23: 'C::func' declared here
>   Line 22: candidate constructor (the implicit copy constructor) not viable: no known conversion from 'B::B' to 'const C::C &' for 1st argument
>   Line 23: passing argument to parameter here
> 4 errors generated.
> 
> Ideas?
> 
> - Kaelyn
> 
> 
> 
> <ns-pt2-20110627.diff>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110628/9dd33321/attachment.html>