[PATCH] Offer typo suggestions for 'using'.

[Luke Zarko]

Thanks for the feedback!

1- I've removed the guard.
2- Done. I've also added a FixIt to remove using decls that typo
correction claims have unnecessary specifiers.
3- Done; I believe I understand what droppedSpecifier means, but
there's a few tests I added in test/SemaCXX/using-decl-1.cpp that seem
to show that it's maybe not making all the corrections it might be
able to (is droppedSpecifier all-or-nothing?)

On Mon, Jul 8, 2013 at 11:14 AM, Kaelyn Uhrain <rikka at google.com> wrote:
> Luke,
>
> I just looked through the patch and have a few questions/comments about it:
>
> 1) Why do you only perform typo correction when the scope is not NULL? I did
> not see any issues within tools/clang/test/ with the guard removed. Plus, as
> I mentioned on IRC, there is only one path in typo correction that
> dereferences the scope without checking it is not NULL, that path seems to
> be a hard-to-reach path and/or irrelevant in the context you are running it
> (iirc, it was only reached from a very ObjC-specific code path), and is most
> likely a bug in the code.
>
> 2) Is there a reason you aren't providing a fixit hint for the typo
> correction? If so, why can't one be provided? (And in the case that a fixit
> cannot be provided, you need to drop the declaration of CorrectedStr since
> it isn't being used.) If a fixit hint can be provided, please do and add a
> test for the fixit hint to an appropriate file under test/FixIt/; basically
> you just need to put one of your tests that offers a correction under there
> to make sure that the fixit hint provides the right substitution in the
> right place and doesn't introduce additional compilation errors.
>
> 3) You'll need to update your patch against clang HEAD and make sure your
> tests still run, since last week I submitted some typo correction changes
> and diag::err_no_member_suggest takes an additional argument to support
> dropping namespace qualifiers (look for declarations of "bool
> droppedSpecifier", e.g. in TryNamespaceTypoCorrection in SemaDeclCXX.cpp or
> in Sema::BuildCXXNestedNameSpecifier in SemaCXXScopeSpec.cpp, for examples
> of how to determine if the correction is dropping the name specifier but not
> changing the base identifier).
>
> Cheers,
> Kaelyn
>
>
> On Mon, Jul 8, 2013 at 10:28 AM, Luke Zarko <zarko at google.com> wrote:
>>
>> Ping--is there still something that needs to be done to this?
>>
>>
>> On Wed, Jun 26, 2013 at 3:04 PM, Luke Zarko <zarko at google.com> wrote:
>> > Thanks for looking it over!
>> >
>> > I've attached an updated patch with the indent fix (clang-format
>> > wanted the extra two spaces for some reason) and tests for
>> > IsTypeName/isa<TypeDecl> combinations. I moved the tests to
>> > using-decl-1.cpp; they seemed more appropriate there.
>> >
>> > It's unclear how to get into the IsInstantiation case, since the
>> > callers of BuildUsingDeclaration always seem to pass in 0 for Scope
>> > when they pass true for instantiation
>> > (TemplateDeclInstantiator::VisitUnresolvedUsingTypenameDecl,
>> > TemplateDeclInstantiator::VisitUnresolvedUsingValueDecl). Using
>> > getScopeForContext(CurContext) to try and get at a Scope when I need
>> > it for CorrectTypo returned null. UsingValidatorCCC's checks should be
>> > logically equivalent to the checks subsequent in the source file.
>> >
>> > On Wed, Jun 26, 2013 at 12:41 PM, Richard Smith <richard at metafoo.co.uk>
>> > wrote:
>> >> On Wed, Jun 26, 2013 at 12:35 PM, Luke Zarko <zarko at google.com> wrote:
>> >>> I found a case where Clang doesn't yet help with typo correction:
>> >>>
>> >>> tests/base_using.cc:
>> >>>
>> >>> namespace Q { class AAA {}; }
>> >>> using Q::AAB;
>> >>>
>> >>> Before:
>> >>>
>> >>> tests/base_using.cc:2:10: error: no member named 'AAB' in namespace
>> >>> 'Q'
>> >>> using Q::AAB;
>> >>>       ~~~^
>> >>>
>> >>> After:
>> >>>
>> >>> tests/base_using.cc:2:10: error: no member named 'AAB' in namespace
>> >>> 'Q'; did you mean 'AAA'?
>> >>> using Q::AAB;
>> >>>       ~~~^
>> >>> tests/base_using.cc:1:21: note: 'AAA' declared here
>> >>> namespace Q { class AAA {}; }
>> >>>                     ^
>> >>
>> >> Code change looks good, but could do with more test coverage for the
>> >> type name / non-type-name / in instantiation cases.
>> >>
>> >> +        Diag(R.getNameLoc(), diag::err_no_member_suggest)
>> >> +            << NameInfo.getName() << LookupContext <<
>> >> CorrectedQuotedStr
>> >> +            << SS.getRange();
>> >>
>> >> We typically indent << only two spaces, not four (clang-format bug?).
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: using-typo-v3.patch
Type: application/octet-stream
Size: 13706 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130709/0467d3b0/attachment.obj>