patch: remove the unqualified name cache for typos

[Kaelyn Takata]

In general this patch looks good. And it is probably time to kill the cache
anyway now that so much of the typo correction bypasses it anyway
(correction callbacks that implement a meaningful ValidateCandidate, and
the delayed typo correction for the most part). A few things that should be
cleaned up though:

1) The declaration of EmptyTypoCorrection
in Sema::makeTypoCorrectionConsumer is unused,

2) The IsUnqualifiedLookup can be dropped from Sema::FailedCorrection now
that it is unused within the function,

3) The EmptyTypoCorrection and ValidatingCallback variables
in Sema::CorrectTypo can be dropped now that the third argument to
Sema::FailedCorrection is unused, and

4) IsUnqualifiedLookup can be made a local variable
within Sema::makeTypoCorrectionConsumer instead of a out-parameter and the
corresponding variables in CorrectTypo and CorrectTypoDelayed can be
removed, now that CorrectTypo doesn't need to know whether the lookup is
qualified or not (which was simply passed along to various calls to
FailedCorrection).

However, as this patch is already fairly large (removing the cache, adding
the new flag, and making use of it to reunify the two main typo correction
test files) I'm fine with 2-4 being done in a separate cleanup commit.

The only concern I have with this patch is actually incidental to it:
in test/CXX/class.access/class.friend/p11.cpp that the notes from the newly
exposed suggestions are wrong--they are pointing to previous friend
declarations (which had also been typo-corrected) instead of to the actual
declarations of ::test2::foo and ::test2::bar. I suspect this is a
relatively simple fix, probably a matter using the the
primary/canonical/first delcaration instead of simply the most recent one
for the NamedDecl descendants that are also Redeclarable (such as
FunctionDecl and RecordDecl), though it is also an important one from a QoI
perspective as that test demonstrates the "XYZ declared here" pointing to
incorrect and potentially confusing locations.

On Sat, Dec 13, 2014 at 2:47 AM, Nick Lewycky <nlewycky at google.com> wrote:

> This patch removes Sema::UnqualifiedTyposCorrected.
>
> I'm not sure this is the right thing to do. As far as I can tell this
> cache can't be made to be correct (wouldn't we need to invalidate it every
> time a new decl is declared?) and I'm not convinced it saves us much
> compile time. However, I'm not confident in my understanding of the code,
> so please do push back if it is beneficial.
>
> One bug it fixes is a case where the same full-expression has the two
> typos in the same identifier, and we emit the correction at the SourceRange
> for the first typo twice instead of once for each place it's misspelled.
> This is because the SourceRange is on the TypoCorrection, and the cache key
> is an IdentifierInfo. Adding the SourceRange to the cache key wouldn't make
> much sense because we wouldn't ever re-use it. (NB, I have similar feelings
> about TypoCorrectionFailures which I am not touching in this patch. Why
> would we do typo correction twice at the same location?)
>
> Removing it improves typo correction quality and there are changes to the
> tests (see p11.cpp, and the last two tests in typo-correction) where it
> fired. Because the cutoff for typos is a function of this cache, I move
> that to a flag (-fspell-checking-limit=...) and turn the limit off entirely
> to merge typo-correction-pt2.cpp and typo-correction.cpp. Also turn it up
> from 20 to 50 to accommodate existing tests. Without caching, we run up the
> counter more quickly.
>
> Please review!
>
> Nick
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141215/550d6d5c/attachment.html>