[PATCH] Offer typo suggestions for 'using'.

[Kaelyn Uhrain]

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 --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130708/9b44a292/attachment.html>