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

[Kaelyn Uhrain]

On Wed, Jun 22, 2011 at 1:23 PM, Douglas Gregor <dgregor at apple.com> wrote:

>
> On Jun 22, 2011, at 10:48 AM, Kaelyn Uhrain wrote:
>
> > So in the interest of keeping the overall changes a bit more digestible,
> I've split my dev branch in two. The first part, for which I've attached the
> latest diff, contains just the changes for the improved version of
> Sema::CorrectTypo and does not include the new unit test or any changes to
> any of the callers of CorrectTypo (all calls are going through the
> compatibility wrapper, and all unit tests pass). The second part, which I'm
> still working on (and which is already a third of the combined patch size),
> already has many of the callers modified to use the new version of
> CorrectTypo so I'd be fine with getting the first part submitted. Splitting
> it apart should also make the changes a bit easier to bisect if something
> breaks down the road.
>
> Thanks for breaking this up. However, I'd like to have at least one test in
> the initial patch, so that we know that things are starting to work. Then
> follow-on commits/patches can switch over call sites to get the improved
> typo correction everywhere.
>

Well the catch is that until at least one of the call sites are switched to
the new version of CorrectTypo, the code would at best give an
incorrect/incomplete suggestion--it would suggest using an identifier from
another namespace without the proper qualifiers to make the identifier
visible, so the suggestion would (in the fixit case) end up tripping another
warning.  If you'd like I could fix up one of the CorrectTypo callers and
add stripped-down version of
missing-namespace-qualifier-typo-corrections.cpp that only tests that one
instance.


> A few small comments/questions:
>
> +class SpecifierInfo {
> + public:
> +  DeclContext* declContext;
> +  NestedNameSpecifier* nameSpecifier;
> +  unsigned editDistance;
> +
> +  SpecifierInfo(DeclContext *Ctx, NestedNameSpecifier *NNS, unsigned ED)
> +      : declContext(Ctx), nameSpecifier(NNS), editDistance(ED) {}
> +};
>
> Public fields should start with an uppercase letter, per the LLVM coding
> standards.
>

Whoops.  Fixed.


>
> +    // If there are results in the closest bucket, stop
> +    if (!DI->second.empty()
> +        /* TODO: Fix ObjC tests (in particular SemaObjC/ivar-ref-misuse.m)
> */
> +        || (getLangOptions().ObjC1 && ED > 0))
> +      break;
>
> Is this still an issue to be resolved?
>

I'll test it and let you know. ;)


>
> +    // Only perform the qualified lookups for C++
> +    // 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?
>

Nope.  I've removed the FIXME.


>
> ... and one larger issue, which is that I'm seeing an infinite loop on this
> test case:
>
> #include <vector>
> #include <algorithm>
>
> int main() {
>  std::vector<int> v;
>  (sort)(v.begin(), v.end());
> }
>
> in the typo-correction logic.
>

Interesting... looking into it.

Thanks!
Kaelyn
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110622/35acc3e6/attachment.html>