On Thu, Oct 23, 2014 at 11:14 AM, Kaelyn Takata <[email protected]> wrote:
> Well, I've found a test case that would trigger the assertion but in the
> code's current form does the right thing:
>
Great!
>
> class Foo {
> public:
> template <> void testIt();
> void textIt();
> };
>
> void test(Foo *f) {
> f->TestIt();
> }
>
>
> With the patchset in it's current incarnation (including the fix I
> mentioned in my last email):
>
I didn't quite follow the description of the other fix/issue there - few
too many patches in my head. Could you send the latest version of this
patch under review?
>
> fail.cc:3:20: error: no function template matches function template
> specialization 'testIt'
> template <> void testIt();
> ^
> fail.cc:8:6: error: no member named 'TestIt' in 'Foo'; did you mean
> 'textIt'?
> f->TestIt();
> ^~~~~~
> textIt
> fail.cc:4:8: note: 'textIt' declared here
> void textIt();
> ^
> 2 errors generated.
>
>
> With the "if (!NE.isInvalid())" as an assertion:
>
> fail.cc:3:20: error: no function template matches function template
> specialization 'testIt'
> template <> void testIt();
> ^
> clang-3.6: ../tools/clang/lib/Sema/SemaExprCXX.cpp:6055: ExprResult
> (anonymous namespace)::TransformTypos::TransformTypoExpr(clang::TypoExpr
> *): Assertion `!NE.isInvalid()' failed.
>
>
> Note that the (invalid) function template "testIt" has an edit distance of
> 1 from "TestIt" and the proper suggestion, "textIt", has an edit distance
> of 2. I've added the above test case to typo-correction-delayed.cpp.
>
>
> On Thu, Oct 23, 2014 at 11:05 AM, David Blaikie <[email protected]>
> wrote:
>
>> (sorry for the repetition - I'm failing at 'reply all' this morning)
>>
>> On Thu, Oct 23, 2014 at 10:53 AM, Kaelyn Takata <[email protected]> wrote:
>>
>>> On Wed, Oct 22, 2014 at 2:19 PM, David Blaikie <[email protected]>
>>> wrote:
>>>
>>>>
>>>>
>>>> On Tue, Oct 21, 2014 at 1:37 PM, Kaelyn Takata <[email protected]>
>>>> wrote:
>>>>
>>>>>
>>>>>
>>>>> On Fri, Oct 17, 2014 at 9:52 AM, David Blaikie <[email protected]>
>>>>> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On Thu, Oct 9, 2014 at 10:12 AM, David Blaikie <[email protected]>
>>>>>> wrote:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Thu, Oct 9, 2014 at 9:58 AM, Kaelyn Takata <[email protected]>
>>>>>>> wrote:
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Tue, Oct 7, 2014 at 5:03 PM, David Blaikie <[email protected]>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Tue, Oct 7, 2014 at 2:29 PM, Kaelyn Takata <[email protected]>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Tue, Oct 7, 2014 at 11:43 AM, David Blaikie <
>>>>>>>>>> [email protected]> wrote:
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Mon, Oct 6, 2014 at 3:46 PM, Kaelyn Takata <[email protected]>
>>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> On Fri, Oct 3, 2014 at 11:45 AM, David Blaikie <
>>>>>>>>>>>> [email protected]> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Given that these are parts of a patch series, some of which
>>>>>>>>>>>>> have already been signed off on/I haven't looked at, it's a bit
>>>>>>>>>>>>> hard to
>>>>>>>>>>>>> review the overall design - but I believe you & Richard talked
>>>>>>>>>>>>> that over,
>>>>>>>>>>>>> so I'm not too worried about that (just explaining why my review
>>>>>>>>>>>>> feedback's
>>>>>>>>>>>>> going to be a bit narrow)
>>>>>>>>>>>>>
>>>>>>>>>>>>> + if (NumTypos > 0 && !ExprEvalContexts.empty())
>>>>>>>>>>>>> + ExprEvalContexts.back().NumTypos += NumTypos;
>>>>>>>>>>>>> + else
>>>>>>>>>>>>> + assert(NumTypos == 0 && "There are outstanding typos after
>>>>>>>>>>>>> popping the "
>>>>>>>>>>>>> + "last
>>>>>>>>>>>>> ExpressionEvaluationContextRecord");
>>>>>>>>>>>>>
>>>>>>>>>>>>> Could this be simplified? I think it's equivalent to:
>>>>>>>>>>>>>
>>>>>>>>>>>>> if (!ExprEvalContexts.empty())
>>>>>>>>>>>>> ExprEvalContexts.back().NumTypos += NumTypos;
>>>>>>>>>>>>> else
>>>>>>>>>>>>> assert(NumTypos == 0 ... );
>>>>>>>>>>>>>
>>>>>>>>>>>>> or:
>>>>>>>>>>>>>
>>>>>>>>>>>>> assert(ExprEvalContexts.empty() || NumTypos != 0 && ...)
>>>>>>>>>>>>> if (!ExprEvalContexts.empty())
>>>>>>>>>>>>> ...
>>>>>>>>>>>>>
>>>>>>>>>>>>> Yes, the "NumTypos > 0" was unnecessary and has been removed.
>>>>>>>>>>>> (FYI, the second form you suggested doesn't quite work because the
>>>>>>>>>>>> assert
>>>>>>>>>>>> would fail on the common case where ExprEvalContexts is not empty
>>>>>>>>>>>> and there
>>>>>>>>>>>> are no typos.)
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Hmm, right, maybe:
>>>>>>>>>>>
>>>>>>>>>>> assert(ExprEvalContexts.empty() ^ NumTypos != 0 && ... )
>>>>>>>>>>> if (!ExprEvalContexts.empty())
>>>>>>>>>>> ExprEvalContexts.back().NumTypos += NumTypos;
>>>>>>>>>>>
>>>>>>>>>>> but perhaps that's just too confusing. I just find an assertion
>>>>>>>>>>> as the only thing inside a block of code to be a bit off... but it's
>>>>>>>>>>> certainly not unprecedented in LLVM and is probably more legible -
>>>>>>>>>>> just my
>>>>>>>>>>> own hangup to get over :)
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> There's an unnecessary semicolon at the end of the "while (true)"
>>>>>>>>>>>> loop in TransformTypos::Transform
>>>>>>>>>>>>>
>>>>>>>>>>>>> Removed.
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> I'd roll this up a bit:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> + auto &State = SemaRef.getTypoExprState(TE);
>>>>>>>>>>>>> + if (State.DiagHandler)
>>>>>>>>>>>>> +
>>>>>>>>>>>>> (*State.DiagHandler)(State.Consumer->getCurrentCorrection());
>>>>>>>>>>>>>
>>>>>>>>>>>>> into:
>>>>>>>>>>>>>
>>>>>>>>>>>>> if (auto *Handler = SemaRef.getTypoExprState(TE).DiagHandler)
>>>>>>>>>>>>> (*DiagHandler)(State.Consumer->getCurrentCorrection());
>>>>>>>>>>>>>
>>>>>>>>>>>>> That rollup doesn't work because State would be unknown when
>>>>>>>>>>>> accessing State.Consumer in the argument to the DiagHandler.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Oh, right!
>>>>>>>>>>>
>>>>>>>>>>>> count in the condition, plus insert in the body of the if in
>>>>>>>>>>>> TransformTypos::TransformTypoExpr would be more efficient as a
>>>>>>>>>>>> single insert-and-check (by avoiding two separate lookups into the
>>>>>>>>>>>> set):
>>>>>>>>>>>>> if (TypoExprs.insert(E).second) // means this element
>>>>>>>>>>>>> previously didn't exist in the set, and now does
>>>>>>>>>>>>>
>>>>>>>>>>>>> I've changed the count-then-insert to just an insert since
>>>>>>>>>>>> SmallPtrSetImpl::insert returns true when the item wasn't already
>>>>>>>>>>>> in the
>>>>>>>>>>>> set.
>>>>>>>>>>>>
>>>>>>>>>>>>> This:
>>>>>>>>>>>>>
>>>>>>>>>>>>> TransformCache.find(E) != TransformCache.end()
>>>>>>>>>>>>>
>>>>>>>>>>>>> might commonly be written as:
>>>>>>>>>>>>>
>>>>>>>>>>>>> TransformCache.count(E)
>>>>>>>>>>>>>
>>>>>>>>>>>>> Optionally with "!= 0", but often count is used directly as a
>>>>>>>>>>>>> boolean test, especially when it's a single set/map (not a multi
>>>>>>>>>>>>> set/map).
>>>>>>>>>>>>>
>>>>>>>>>>>>> Changed.
>>>>>>>>>>>>
>>>>>>>>>>>>> This comment didn't quite make sense to me:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> + // If corrections for the first TypoExpr have been
>>>>>>>>>>>>> exhausted, retry them
>>>>>>>>>>>>> + // against the next combination of substitutions for all
>>>>>>>>>>>>> of the other
>>>>>>>>>>>>> + // TypoExprs.
>>>>>>>>>>>>>
>>>>>>>>>>>>> When you say "retry them" which them do you mean? The corrections
>>>>>>>>>>>>> for the first TypoExpr? But what does it mean for them to be
>>>>>>>>>>>>> exhausted then, if they can still be considered?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Maybe I need a bit of help with the higher level algorithm here -
>>>>>>>>>>>>> I assume the idea is:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Call transform.
>>>>>>>>>>>>> As it visits TypoExpr nodes, it tries their first typo candidate
>>>>>>>>>>>>> available.
>>>>>>>>>>>>> If we get to the end and have invalid code, reset the state of
>>>>>>>>>>>>> all the typo corrections except the first one (on its next query,
>>>>>>>>>>>>> it'll give back its second alternative).
>>>>>>>>>>>>>
>>>>>>>>>>>>> Here is where the confusion is at. As the transform visits
>>>>>>>>>>>> each TypoExpr node it tries the first candidate available. If the
>>>>>>>>>>>> first
>>>>>>>>>>>> candidates of all the TypoExprs don't work, it then tries the next
>>>>>>>>>>>> (and
>>>>>>>>>>>> subsequent) candidates of only the first TypoExpr that was seen.
>>>>>>>>>>>> When it
>>>>>>>>>>>> exhausts the candidates from the first TypoExpr, it then resets
>>>>>>>>>>>> the stream
>>>>>>>>>>>> of candidates for the first TypoExpr and fetches the next
>>>>>>>>>>>> candidate of the
>>>>>>>>>>>> TypoExpr, continuing in this way until all combinations of
>>>>>>>>>>>> candidates of
>>>>>>>>>>>> all TypoExprs have been tried.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> So I'm still a bit confused about which parts of the retry logic
>>>>>>>>>>> are in which parts of the code.
>>>>>>>>>>>
>>>>>>>>>>> OK, I think I've got it. Transform is called and we don't even
>>>>>>>>>>> know which TypoExprs are in the Expr. So we transform.
>>>>>>>>>>>
>>>>>>>>>>> As transform runs it comes across TypoExprs, and attempts to
>>>>>>>>>>> transform them. For each one, it tries candidates until it finds a
>>>>>>>>>>> good one
>>>>>>>>>>> (based on the immediate context - what are the cases where
>>>>>>>>>>> BuildDeclarationNameExpr fails here? (when does NE.isInvalid
>>>>>>>>>>> fail?)).
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I think there is a bit of confusion on what a "retry" is. Within
>>>>>>>>>> TransformTypoExpr, for either the first TypoExpr ever encountered by
>>>>>>>>>> this
>>>>>>>>>> TreeTransform or for a TypoExpr without a cached substitution, it
>>>>>>>>>> tries to
>>>>>>>>>> build a valid subexpression from the TypoCorrection candidates (e.g.
>>>>>>>>>> a
>>>>>>>>>> DeclRefExpr) and continues trying candidates until it finds one for
>>>>>>>>>> which a
>>>>>>>>>> valid subexpression can be formed, which it caches and returns. If
>>>>>>>>>> none of
>>>>>>>>>> the candidates worked, it will cache and return an invalid
>>>>>>>>>> ExprResult. The
>>>>>>>>>> TreeTransform then takes the returned subexpression and tries to
>>>>>>>>>> rebuild
>>>>>>>>>> the full expression (possibly trying to transform multiple TypoExprs
>>>>>>>>>> along
>>>>>>>>>> with CallExprs, etc).
>>>>>>>>>>
>>>>>>>>>> I don't remember off the top of my head without looking at the
>>>>>>>>>> body, but I think BuildDeclarationNameExpr will fail in cases where
>>>>>>>>>> the
>>>>>>>>>> correction is e.g. a type name, or a member reference
>>>>>>>>>> name--basically,
>>>>>>>>>> BuildDeclarationNameExpr returns an ExprResult and the correct
>>>>>>>>>> behavior in
>>>>>>>>>> principle is to check for an invalid result and try the next
>>>>>>>>>> correction
>>>>>>>>>> candidate when one is seen--even if the Consumer never returns a
>>>>>>>>>> candidate
>>>>>>>>>> for which BuildDeclarationNameExpr would fail.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> So, in your example below, maybe the right answer is BEH, but AF
>>>>>>>>>>> is valid, with no correct candidate for the 3 choice.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> No, AF with no correction for the 3rd TypoExpr would result in
>>>>>>>>>> the entire Expr result being invalid within the main Transform
>>>>>>>>>> method.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> Consumers start at "before the first" element, calling
>>>>>>>>>>> "getNextCorrection" on a new consumer gives you the first
>>>>>>>>>>> correction.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> That is correct. And the "before the first" element is also an
>>>>>>>>>> empty correction for use with getCurrentCorrection. ;)
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On the first call to TransformExpr, the first call to
>>>>>>>>>>> TransformTypoExpr chooses A, records it, continues.
>>>>>>>>>>> Gets to the second TransformTypoExpr, tries D and E, succeeds at
>>>>>>>>>>> F.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> it would only try D then E then F within a single call to
>>>>>>>>>> TransformTypoExpr if BuildDeclarationNameExpr failed for D and E.
>>>>>>>>>>
>>>>>>>>>>> On the third TransformTypoExpr, it tries H and I, both fail, so
>>>>>>>>>>> it returns from the last line ("return TransformCache[E] =
>>>>>>>>>>> ExprError]).
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Same case here--that last line would only be reached if H and I
>>>>>>>>>> both failed within the call to BuildDeclarationNameExpr.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> We get back to Transform with a failure. So we walk the
>>>>>>>>>>> TypoExprs (in CheckAndAdvanceTypoExprCorrectionStreams) we reset
>>>>>>>>>>> the 3rd
>>>>>>>>>>> consumer (because it was finished).
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> No, CheckAndAdvanceTypoExprCorrectionStreams would do nothing if
>>>>>>>>>> the first TypoExpr is not finished. If the first TypoExpr is
>>>>>>>>>> finished, then
>>>>>>>>>> it would reset it and clear the cached result for the second
>>>>>>>>>> TypoExpr (so
>>>>>>>>>> that the next call to TransformTypoExpr on the second TypoExpr would
>>>>>>>>>> try a
>>>>>>>>>> new candidate instead of using the cached result). Only if both the
>>>>>>>>>> first
>>>>>>>>>> and second TypoExprs were *both* finished would the cached value of
>>>>>>>>>> the
>>>>>>>>>> third TypoExpr be cleared (and the candidate streams for both the
>>>>>>>>>> first and
>>>>>>>>>> second TypoExprs would be reset in addition to their cached results
>>>>>>>>>> being
>>>>>>>>>> removed from the cache). The only way
>>>>>>>>>> CheckAndAdvanceTypoExprCorrectionStreams would see if the third
>>>>>>>>>> TypoExpr
>>>>>>>>>> was finished would be if both the first and second TypoExprs were
>>>>>>>>>> also
>>>>>>>>>> finished, and upon resetting it the tree transform would only be
>>>>>>>>>> attempted
>>>>>>>>>> again if there were at least four known TypoExprs (i.e. the number
>>>>>>>>>> of known
>>>>>>>>>> TypoExprs is greater than the number of TypoExprs that were finished
>>>>>>>>>> and
>>>>>>>>>> had to be reset by CheckAndAdvanceTypoExprCorrectionStreams).
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> We transform again:
>>>>>>>>>>>
>>>>>>>>>>> First typo) We are at A so we call next and get B
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Yes. The first TypoExpr would always have it's next candidate
>>>>>>>>>> tried by a call to TransformTypoExpr.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> Second typo) From F we continue and try G, fail, then we're
>>>>>>>>>>> finished for the second typo.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> No, G would only be tried if the result from transforming F was
>>>>>>>>>> removed from the cache.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> Third typo) Perhaps we find some solution for this, or not, it
>>>>>>>>>>> doesn't matter.
>>>>>>>>>>>
>>>>>>>>>>> We get back to Transform with a failure (second typo), So we
>>>>>>>>>>> walk the TypoExprs again, resetting the second (and maybe the
>>>>>>>>>>> third).
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> If there are only three TypoExprs, then resetting the third
>>>>>>>>>> TypoExpr would mean there are no valid combinations of corrections
>>>>>>>>>> that
>>>>>>>>>> yield a valid expression (where the final result of the transform is
>>>>>>>>>> neither an invalid ExprResult nor caused any errors to be reported
>>>>>>>>>> by the
>>>>>>>>>> SFINAETrap).
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>> First typo) We are at B so we call next and get to C, let's say
>>>>>>>>>>> we fail here - C isn't even remotely valid.
>>>>>>>>>>> Now it really matters whether we find/fail on the second and
>>>>>>>>>>> third typo. If we fail on the second and third... we're going to
>>>>>>>>>>> give up.
>>>>>>>>>>> We'll find that all the corrections are finished and we'll conclude
>>>>>>>>>>> that
>>>>>>>>>>> we've tried everything.
>>>>>>>>>>>
>>>>>>>>>>> Am I following this right?
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Not quite. I think you are forgetting about the fact that, for
>>>>>>>>>> all TypoExprs except the first, a cached result is used if it exists
>>>>>>>>>> instead of fetching a new candidate from the Consumer and trying to
>>>>>>>>>> build a
>>>>>>>>>> valid Expr with it.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Ah, looking at the code again I see what you mean. The 'break' is
>>>>>>>>> the bit that I missed in the reset/advance loop.
>>>>>>>>>
>>>>>>>>> Which patches would I have to apply to ToT to be able to exercise
>>>>>>>>> this code? It might help for me to step through some of these
>>>>>>>>> scenarios in
>>>>>>>>> a debugger.
>>>>>>>>>
>>>>>>>>
>>>>>>>> To exercise this code, you'd need the full patch series (all 8) to
>>>>>>>> exercise this code because it's not until patch #8 that anything
>>>>>>>> actually
>>>>>>>> generates TypoExprs in the AST. I can send you a combined patch
>>>>>>>> rebased on
>>>>>>>> ToT off-list if you'd like (it's ~112KB uncompressed), and the new test
>>>>>>>> cases in test/SemaCXX/typo-correction-delayed.cpp explicitly test this
>>>>>>>> functionality.
>>>>>>>>
>>>>>>>
>>>>>>> That'd be great!
>>>>>>>
>>>>>>
>>>>>> With your total patch applied I've been able to reproduce a few typo
>>>>>> correction scenarios - thanks!
>>>>>>
>>>>>> But I can't seem to reach/exercise the loop in
>>>>>> TransformTypos::TransformTypoExpr - if I change the "if
>>>>>> (!NE.isInvalid())"
>>>>>> condition at the end of the loop (the only way to repeat the body of the
>>>>>> loop) to "assert(!NE.isInvalid())" it doesn't fire on any of the test
>>>>>> cases
>>>>>> (nor on my own modified versions). Do you have some tests that cover
>>>>>> this?
>>>>>> Could you show/describe a simple situation where it occurs so I can play
>>>>>> around with that part of the logic?
>>>>>>
>>>>>
>>>>> I'm not sure it can be triggered with CorrectTypoDelayed only hooked
>>>>> up to LookupMemberExprInRecord (a static function called by
>>>>> Sema::BuildMemberReferenceExpr directly and indirectly via
>>>>> LookupMemberExpr), but in the second patch set which hooks
>>>>> CorrectTypoDelayed up to DiagnoseEmptyLookup, the assert is triggered by
>>>>> the tests SemaCXX/type-definition-in-specifier.cpp
>>>>> and SemaCXX/warn-thread-safety-parsing.cpp (in addition to the loop being
>>>>> the correct/principled behavior since there is no guarantee that the
>>>>> recovery callback will generate a valid ExprResult from a non-empty
>>>>> TypoCorrection). For this patch set, to trigger the assert a code snippet
>>>>> would have to correct a class member lookup and have a candidate that is a
>>>>> member function, constant, variable, or template in the current class or
>>>>> one of its base classes (per the RecordMemberExprValidatorCCC) yet which
>>>>> yields causes MemberExprTypoRecovery to return an ExprError when trying to
>>>>> build the member reference. Basically, it would require having the typo
>>>>> correction try a candidate that is an accessible class member that cannot
>>>>> be reference--something that I'm not sure is even possible in C++.
>>>>>
>>>>
>>>> Hrm, OK - could you change it to an assertion (& remove the outer
>>>> loop/replace it with a conditional) until that later patch where it becomes
>>>> necessary, so it'll be easier to review it in the context of it being
>>>> executed/used code?
>>>>
>>>
>>> I understand your suggestion, however I don't think changing the code to
>>> do the wrong thing and assert if a false assumption is violated is the
>>> right thing to do. If the only source of an ExprResult in the loop was the
>>> call to Sema::BuildDeclarationNameExpr, I'd be a bit more willing to do
>>> so... but if a recovery handler callback is provided and invoked, there's
>>> no guarantee it won't return an invalid ExprResult, and no documentation of
>>> the assumption that the callback must return a valid ExprResult (outside of
>>> the one assertion you are suggesting, buried within the TypoExpr handling
>>> machinery). Additionally, changing the code to do the wrong thing by
>>> changing the "while (...)" to "if (...)" and replacing "if
>>> (!NE.isInvalid())" with a bogus assertion does nothing to simplify the
>>> actual code block... particularly since it would immediately be changed
>>> right back to the way it is now in the next patch set (a version of which I
>>> had already mailed out two months ago).
>>>
>>
>> My concern is that this is untested (& actually untestable/dead in this
>> revision) code - if someone were to sync to this revision and try to
>> understand what the code did, why it was there, and how it should be
>> modified to do other things, they'd be confused & probably get it wrong
>> (because there are no tests to demonstrate the motivation). It's nice for
>> code to go along with the use - the usual rules of providing tests along
>> with code.
>>
>> But I'll muddle through & try to remember to figure out the tests & the
>> design when you commit/review the next patches if necessary.
>>
>>
>>>
>>> (FYI, I also just noticed I had a mistake in the full patch I sent
>>> you... specifically I seem to have forgotten to delete a return statement
>>> when I changed the code to look up the TypoExpr in the TransformCache only
>>> one and assign the result to the created entry and rebased the rest of the
>>> patch set:
>>>
>>> diff --git a/lib/Sema/SemaExprCXX.cpp b/lib/Sema/SemaExprCXX.cpp
>>> index 81b3b01..9e05e82 100644
>>> --- a/lib/Sema/SemaExprCXX.cpp
>>> +++ b/lib/Sema/SemaExprCXX.cpp
>>> @@ -6049,8 +6049,6 @@ public:
>>> if (!TC.isKeyword())
>>> R.addDecl(TC.getCorrectionDecl());
>>> NE = SemaRef.BuildDeclarationNameExpr(CXXScopeSpec(), R, false);
>>> - return TransformCache[E] =
>>> - SemaRef.BuildDeclarationNameExpr(CXXScopeSpec(), R,
>>> false);
>>> }
>>> assert(!NE.isUnset() &&
>>> "Typo was transformed into a valid-but-null ExprResult");
>>>
>>> So the "if (!NE.isUnset())" is intended to be the only early return
>>> within the loop, though it still does not fail as an assert within the
>>> patchset.)
>>>
>>>>
>>>> Without that wrinkle, I think I've followed the code enough to sign off
>>>> on it. Please commit.
>>>>
>>>> [I do find the way the algorithm visits the corrections to be a little
>>>> hard to follow, in that its split between the TypoExprs cache, consumer
>>>> state, TransformTypoExpr, and CheckAndAdvanceTypoExprCorrectionStreams - in
>>>> some very abstract theory I would like having each sequence of typo
>>>> candidates (the Consumers represent this) expose an iterator pair, then a
>>>> generic utility to do combinations of a list of ranges and it would expose
>>>> an iterator over those combinations which you could just increment to get
>>>> the next one - it'd separate out the combination logic into a reusable
>>>> component that could be easily examined/tested, then composed with the typo
>>>> correction stuff to keep the logic there relatively simple/clearer.
>>>>
>>>> I might try to work up an example of what I mean]
>>>>
>>>>
>>>>
>>>>
>>>>>
>>>>>> - David
>>>>>>
>>>>>> (micro review comment I saw along the way: I /think/
>>>>>> CheckAndAdvanceTypoExprCorrectionStreams
>>>>>> could be simplified as follows:
>>>>>>
>>>>>
>>>>> You're right, it can be simplified now that the loop is in its own
>>>>> function. Changed.
>>>>>
>>>>>>
>>>>>> bool CheckAndAdvanceTypoExprCorrectionStreams() {
>>>>>> for (auto TE : TypoExprs) {
>>>>>> TransformCache.erase(TE);
>>>>>> auto &State = SemaRef.getTypoExprState(TE);
>>>>>> if (!State.Consumer->finished())
>>>>>> return true;
>>>>>> State.Consumer->resetCorrectionStream();
>>>>>> }
>>>>>> return false;
>>>>>> }
>>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>> (I'm sort of wondering if this might be better expressed by some
>>>>>>>>> iterator scheme, but I'm not sure - there's no pretty next_combination
>>>>>>>>> algorithm (someone proposed one to boost at some point, it seems, but
>>>>>>>>> that's about as close as it's got to anything common/generic) anyway)
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> So for 3 TypoExprs... let's say 1 has the candidates (A, B, C),
>>>>>>>>>>>> 2 has the candidates (D, E, F, G) and 3 has the candidates (H, I)
>>>>>>>>>>>> the
>>>>>>>>>>>> iterations would test the combinations in order:
>>>>>>>>>>>>
>>>>>>>>>>>> A D H
>>>>>>>>>>>> B D H
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I suppose a simpler form of my above example is: if, on the
>>>>>>>>>>> first pass, we get ADH, on the next pass the code as-is would
>>>>>>>>>>> result in
>>>>>>>>>>> BCI, wouldn't it? It'll "next" each of the corrections.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> (I think you meant BEI, and C is only a candidate of the first
>>>>>>>>>> TypoExpr, not the second one.) No it won't because of the caching.
>>>>>>>>>> The
>>>>>>>>>> second TypoExpr is only effectively "next"ed once all of the
>>>>>>>>>> candidates for
>>>>>>>>>> the first TypoExpr have been tried with the current candidate of the
>>>>>>>>>> second
>>>>>>>>>> TypoExpr, and the third TypoExpr would only be "next"ed if all of the
>>>>>>>>>> candidates of both the first and second TypoExpr have been tried
>>>>>>>>>> with the
>>>>>>>>>> current candidate of the third TypoExpr.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> And I'm still not quite understanding why there's the E !=
>>>>>>>>>>> TypoExprs[0] condition... maybe that's part of what I'm missing. It
>>>>>>>>>>> looks
>>>>>>>>>>> like that code would cause, in a single Transform, multiple
>>>>>>>>>>> instances of
>>>>>>>>>>> the first typo to be computed with different/subsequent typo
>>>>>>>>>>> results...
>>>>>>>>>>> which seems confusing/strange. (though I don't fully understand
>>>>>>>>>>> how/where
>>>>>>>>>>> multiple instances of the same TypoExpr will be visited in a single
>>>>>>>>>>> Transform)
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> That condition ensures that the cached result is never used for
>>>>>>>>>> the first TypoExpr (instead of having an extra conditional for all
>>>>>>>>>> of the
>>>>>>>>>> return statements below here to not cache the result for the first
>>>>>>>>>> TypoExpr).
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> (some nitpicks of the code - there's a duplicate lookup of
>>>>>>>>>>> TransformCache (count then [], or count then insert further down the
>>>>>>>>>>> algorithm - it should probably just be:
>>>>>>>>>>>
>>>>>>>>>>> ExprResult &CacheEntry = TransformCache[E];
>>>>>>>>>>> if (CacheEntry)
>>>>>>>>>>> return CacheEntry;
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> This wouldn't work because "TransformCache[E]" would add a cache
>>>>>>>>>> entry for E, and either the "if (CacheEntry)" would always be true,
>>>>>>>>>> or an
>>>>>>>>>> ExprError() would be considered false and an ExprError cache entry
>>>>>>>>>> would
>>>>>>>>>> always be ignored, both of which would be bad.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> you're right that ExprResult isn't boolean testable - but it is
>>>>>>>>> possible to distinguish ExprError from a default-constructed
>>>>>>>>> ExprResult. A
>>>>>>>>> default constructed ExprResult is in the valid-but-unset state, so
>>>>>>>>> far as I
>>>>>>>>> can tell, so it would look like:
>>>>>>>>>
>>>>>>>>> if (!CacheEntry.isUnset())
>>>>>>>>> return CacheEntry
>>>>>>>>>
>>>>>>>>> (which is, admittedly, not the /most/ legible thing ever)
>>>>>>>>>
>>>>>>>>
>>>>>>>> I'm worried this won't work, or at least would be (potentially)
>>>>>>>> fragile, because .isUnset() is defined as "valid-but-null" and Sema
>>>>>>>> sets
>>>>>>>> valid-but-null as an explicit state in the return values of various
>>>>>>>> functions--at least a few of which I saw while looking at users of the
>>>>>>>> typo
>>>>>>>> correction code.
>>>>>>>>
>>>>>>>
>>>>>>> Ooh... I could totally see that happening (& thanks for the
>>>>>>> explanation).
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Of course, a new local variable could be created containing the
>>>>>>>>>> result of TransformCache.find(E) and then compared to
>>>>>>>>>> TransformCache.end()
>>>>>>>>>> and returning as appropriate, but I find it to be more verbose and
>>>>>>>>>> less
>>>>>>>>>> clear:
>>>>>>>>>>
>>>>>>>>>> auto CacheEntry = TransformCache.find(E);
>>>>>>>>>> if (!TypoExprs.insert(E) && CacheEntry != TransformCache.end())
>>>>>>>>>> return CacheEntry->second;
>>>>>>>>>>
>>>>>>>>>> And neither of the subsequent return statements would be changed
>>>>>>>>>> because the cache entry would still need to be created...
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> If you were using the iterator version, they could be hinted
>>>>>>>>> insertions which might be better - but I agree, if you're just using
>>>>>>>>> the
>>>>>>>>> iterators it's probably not terribly helpful. But as shown above, you
>>>>>>>>> can
>>>>>>>>> actually just use the op[] at the start, check !isUnset, then
>>>>>>>>> initialize
>>>>>>>>> the elements. This is a fairly common idiom in map related code,
>>>>>>>>> though
>>>>>>>>> more often it looks like
>>>>>>>>>
>>>>>>>>
>>>>>>>> After a bit more consideration, I've taken your suggested approach
>>>>>>>> of using .isUnset() and added an assertion on trying to cache an
>>>>>>>> "unset"
>>>>>>>> value ("assert(!NE.isUnset() && ...);").
>>>>>>>>
>>>>>>>
>>>>>>> Sounds like a fair tradeoff - if the assertion does fire we can add
>>>>>>> a test case for it & switch back to something like your original
>>>>>>> formulation.
>>>>>>>
>>>>>>> The other gotcha to be aware of is that since it's a reference into
>>>>>>> a DenseMap are invalidated by insertions - so if any of the work to
>>>>>>> initialize the TransformCache (the while nextCorrection loop, and
>>>>>>> probably
>>>>>>> most importantly the "BuildDeclarationNameExpr" call) can also modify
>>>>>>> the
>>>>>>> TransformCache (by transforming another typo expr) then the reference
>>>>>>> will
>>>>>>> be invalidated and the approach I'd suggested won't be applicable (you
>>>>>>> could still keep just one lookup in the cache hit case, but in the cache
>>>>>>> miss you'd have to do a second lookup after the work)
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> ...
>>>>>>>>>>> return CacheEntry = NE;
>>>>>>>>>>> }
>>>>>>>>>>> return CacheEntry = ExprError();
>>>>>>>>>>>
>>>>>>>>>>> )
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> C D H
>>>>>>>>>>>> A E H
>>>>>>>>>>>> B E H
>>>>>>>>>>>> C E H
>>>>>>>>>>>> A F H
>>>>>>>>>>>> B F H
>>>>>>>>>>>> C F H
>>>>>>>>>>>> A G H
>>>>>>>>>>>> B G H
>>>>>>>>>>>> C G H
>>>>>>>>>>>> A D I
>>>>>>>>>>>> B D I
>>>>>>>>>>>> C D I
>>>>>>>>>>>> A E I
>>>>>>>>>>>> B E I
>>>>>>>>>>>> C E I
>>>>>>>>>>>> A F I
>>>>>>>>>>>> B F I
>>>>>>>>>>>> C F I
>>>>>>>>>>>> A G I
>>>>>>>>>>>> B G I
>>>>>>>>>>>> C G I
>>>>>>>>>>>>
>>>>>>>>>>>>> If we get to the end, have invalid code, and the first typo has
>>>>>>>>>>>>> no alternatives left to try - I guess we just want to print
>>>>>>>>>>>>> errors for everything, without typo corrections?
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>> If all of the combinations of typo corrections failed to create
>>>>>>>>>>>> an acceptable Expr, then the current correction for all of
>>>>>>>>>>>> TypoExprs would
>>>>>>>>>>>> be an empty TypoCorrection which would cause the corresponding
>>>>>>>>>>>> diagnostic
>>>>>>>>>>>> handlers to print out errors without suggestions.
>>>>>>>>>>>>
>>>>>>>>>>>>> It might be helpful to split out some of these chunks (the state
>>>>>>>>>>>>> resetting code and maybe the diagnostic printing at the end) into
>>>>>>>>>>>>> separate functions to give them logical names & make the higher
>>>>>>>>>>>>> level algorithm more clear?
>>>>>>>>>>>>>
>>>>>>>>>>>>> I've split the diagnostics emission and the state handling
>>>>>>>>>>>> into separate helpers, and expanded their description comments.
>>>>>>>>>>>> I'm not
>>>>>>>>>>>> keen on the name for the state handler but it is the best name I
>>>>>>>>>>>> could come
>>>>>>>>>>>> up with that was even remotely descriptive.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Roughly works, though I'm still quite confused, it seems.
>>>>>>>>>>> (alternative name for that function would be "reset" rather than
>>>>>>>>>>> "advance"
>>>>>>>>>>> - but once I understand the algorithm better I might have better
>>>>>>>>>>> suggestions)
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The function removes the cached result for the first TypoExpr
>>>>>>>>>> then checks the TypoExpr's correction candidate stream; if it has
>>>>>>>>>> reached
>>>>>>>>>> the end of the stream, it will reset the stream then perform the
>>>>>>>>>> same check
>>>>>>>>>> for the second TypoExpr, continuing until it either has reset the
>>>>>>>>>> correction streams for all of the TypoExprs or has found a TypoExpr
>>>>>>>>>> that
>>>>>>>>>> hadn't reached the end of its correction stream. And in the former
>>>>>>>>>> case, it
>>>>>>>>>> would signal that all combinations of corrections had been retried
>>>>>>>>>> and the
>>>>>>>>>> TreeTransform should stop searching for a valid combination of
>>>>>>>>>> corrections.
>>>>>>>>>> And now that I think about it, the "E != TypoExprs[0]" is probably
>>>>>>>>>> now a
>>>>>>>>>> superfluous check since calling
>>>>>>>>>> CheckAndAdvanceTypoExprCorrectionStreams
>>>>>>>>>> would always remove the first TypoExpr from the result cache.
>>>>>>>>>>
>>>>>>>>>> The loop at the end ("for (auto E : TypoExprs) {") is over a hashing
>>>>>>>>>> data structure, right? Is that going to cause unstable output of
>>>>>>>>>> diagnostics? (eg: different machines, different memory layout, the
>>>>>>>>>> same program might get the typos printed in a different order) or is
>>>>>>>>>> there something else that will fix that?
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> If you have to change the set to be a SetVector, or similar (to
>>>>>>>>>>>>> stabilize output) - then you won't need the FirstTypoExpr anymore
>>>>>>>>>>>>> - it'll just be the first one in the set, since you iterate in
>>>>>>>>>>>>> insertion order at that point.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Switched it from a SmallPtrSet to a SmallSetVector like it
>>>>>>>>>>>> should have been. Doing so also allowed me to simplify the state
>>>>>>>>>>>> handling
>>>>>>>>>>>> code by being able to assume a deterministic order, and eliminating
>>>>>>>>>>>> FirstTypoExpr.
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> "while (TypoCorrection TC = State.Consumer->getNextCorrection())
>>>>>>>>>>>>> {" doesn't seem to be a loop - it has an unconditional return at
>>>>>>>>>>>>> the end of the body. Should it just be an 'if'?
>>>>>>>>>>>>>
>>>>>>>>>>>>> There was actually a missing check on whether the result of
>>>>>>>>>>>> Sema::BuildDeclarationNameExpr was valid.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Were there missing/added test cases to cover this situation?
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> No because at this point in the patch series, there is nothing
>>>>>>>>>> that actually generates TypoExprs.
>>>>>>>>>>
>>>>>>>>>>> "+ (FullExpr.get()->isTypeDependent() ||
>>>>>>>>>>>>>
>>>>>>>>>>>>> + FullExpr.get()->isValueDependent() ||
>>>>>>>>>>>>> + FullExpr.get()->isInstantiationDependent())) {"
>>>>>>>>>>>>>
>>>>>>>>>>>>> What are those checks for? I don't know what condition we would
>>>>>>>>>>>>> be in if we had typo corrections but it wasn't type, value, or
>>>>>>>>>>>>> instantiation dependent. Perhaps a comment could explain that?
>>>>>>>>>>>>>
>>>>>>>>>>>>> The check is to avoid running the tree transform on Expr trees
>>>>>>>>>>>> that are known to not have a TypoExpr in them, regardless of
>>>>>>>>>>>> whether the
>>>>>>>>>>>> current expression evaluation context still indicates there are
>>>>>>>>>>>> uncorrected
>>>>>>>>>>>> typos. I've added a comment to that effect.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Hmm - I suppose my question might then be: When would we have
>>>>>>>>>>> NumTypos > 0 but be in a situation where the full expression cannot
>>>>>>>>>>> have
>>>>>>>>>>> any typos in it?
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I don't remember off the top of my head because it's been a few
>>>>>>>>>> months now since I wrote the original patches... but basically any
>>>>>>>>>> situation where more than one FullExpr shares an expression
>>>>>>>>>> evaluation
>>>>>>>>>> context or when the current expression evaluation context had its
>>>>>>>>>> NumTypos
>>>>>>>>>> increased by an expression evaluation context that was popped off the
>>>>>>>>>> stack. And TreeTransforms aren't cheap enough to blindly run on
>>>>>>>>>> every Expr
>>>>>>>>>> when NumTypos is non-zero.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> To simplify Sema::clearDelayedTypo you could add
>>>>>>>>>>>> MapVector::erase(const KeyType&) (given that std::map has such an
>>>>>>>>>>>> operation, it seems like a reasonable one for MapVector to have).
>>>>>>>>>>>>>
>>>>>>>>>>>>> Done (simple patch attached).
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Great - if you'd like to commit this separately, it should have
>>>>>>>>>>> a unit test. Otherwise it's probably simple enough to commit along
>>>>>>>>>>> with
>>>>>>>>>>> your new usage if you prefer.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Committed with unit test as r219240.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Sema::getTypoExprState looks like it could be a const function
>>>>>>>>>>>>> (it looks like it's never meant to be called when the element
>>>>>>>>>>>>> isn't already
>>>>>>>>>>>>> in the map) - using 'lookup' instead of operator[] will be a const
>>>>>>>>>>>>> operation on the map (& assert if the element doesn't exist in
>>>>>>>>>>>>> the map).
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Sadly I cannot do that because the operator[] cannot be used if
>>>>>>>>>>>> the method is made const, and .lookup() can't be used because it
>>>>>>>>>>>> returns
>>>>>>>>>>>> by-value instead of by-reference and TypoExprState is not copyable:
>>>>>>>>>>>>
>>>>>>>>>>>> ../tools/clang/lib/Sema/SemaLookup.cpp:4600:10: warning:
>>>>>>>>>>>> returning reference to local temporary object
>>>>>>>>>>>> [-Wreturn-stack-address]
>>>>>>>>>>>> return DelayedTypos.lookup(TE);
>>>>>>>>>>>>
>>>>>>>>>>>> ../include/llvm/ADT/MapVector.h:88:30: error: call to
>>>>>>>>>>>> implicitly-deleted copy constructor of 'const
>>>>>>>>>>>> clang::Sema::TypoExprState'
>>>>>>>>>>>> return Pos == Map.end()? ValueT() :
>>>>>>>>>>>> Vector[Pos->second].second;
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Oh, weird - my mistake in assuming/misremembering what lookup
>>>>>>>>>>> actually does. I'd still make getTypoExprState const and use
>>>>>>>>>>> find+assert
>>>>>>>>>>> just to constrain the interface a bit more rather than leaving it
>>>>>>>>>>> possible
>>>>>>>>>>> to accidentally create a new entry in the map when the API is only
>>>>>>>>>>> intended
>>>>>>>>>>> to query existing entries.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Done. The new changes I've made overall are:
>>>>>>>>>>
>>>>>>>>>> diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h
>>>>>>>>>> index b30bf5e..f644fdd 100644
>>>>>>>>>> --- a/include/clang/Sema/Sema.h
>>>>>>>>>> +++ b/include/clang/Sema/Sema.h
>>>>>>>>>> @@ -2628,7 +2628,7 @@ private:
>>>>>>>>>> bool ErrorRecovery, bool
>>>>>>>>>> &IsUnqualifiedLookup);
>>>>>>>>>>
>>>>>>>>>> public:
>>>>>>>>>> - const TypoExprState &getTypoExprState(TypoExpr *TE);
>>>>>>>>>> + const TypoExprState &getTypoExprState(TypoExpr *TE) const;
>>>>>>>>>>
>>>>>>>>>> /// \brief Clears the state of the given TypoExpr.
>>>>>>>>>> void clearDelayedTypo(TypoExpr *TE);
>>>>>>>>>> diff --git a/lib/Sema/SemaExprCXX.cpp b/lib/Sema/SemaExprCXX.cpp
>>>>>>>>>> index 73aa997..1f08276 100644
>>>>>>>>>> --- a/lib/Sema/SemaExprCXX.cpp
>>>>>>>>>> +++ b/lib/Sema/SemaExprCXX.cpp
>>>>>>>>>> @@ -6028,7 +6028,7 @@ public:
>>>>>>>>>> // If the TypoExpr hasn't been seen before, record it.
>>>>>>>>>> Otherwise, return the
>>>>>>>>>> // cached transformation result if there is one and the
>>>>>>>>>> TypoExpr isn't the
>>>>>>>>>> // first one that was encountered.
>>>>>>>>>> - if (!TypoExprs.insert(E) && E != TypoExprs[0] &&
>>>>>>>>>> TransformCache.count(E)) {
>>>>>>>>>> + if (!TypoExprs.insert(E) && TransformCache.count(E)) {
>>>>>>>>>> return TransformCache[E];
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> diff --git a/lib/Sema/SemaLookup.cpp b/lib/Sema/SemaLookup.cpp
>>>>>>>>>> index 8ddb5fd..bb6c76a 100644
>>>>>>>>>> --- a/lib/Sema/SemaLookup.cpp
>>>>>>>>>> +++ b/lib/Sema/SemaLookup.cpp
>>>>>>>>>> @@ -4601,8 +4601,11 @@ TypoExpr
>>>>>>>>>> *Sema::createDelayedTypo(std::unique_ptr<TypoCorrectionConsumer> TCC,
>>>>>>>>>> return TE;
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> -const Sema::TypoExprState &Sema::getTypoExprState(TypoExpr *TE) {
>>>>>>>>>> - return DelayedTypos[TE];
>>>>>>>>>> +const Sema::TypoExprState &Sema::getTypoExprState(TypoExpr *TE)
>>>>>>>>>> const {
>>>>>>>>>> + auto Entry = DelayedTypos.find(TE);
>>>>>>>>>> + assert(Entry != DelayedTypos.end() &&
>>>>>>>>>> + "Failed to get the state for a TypoExpr!");
>>>>>>>>>> + return Entry->second;
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> void Sema::clearDelayedTypo(TypoExpr *TE) {
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>> Otherwise this all looks like rather neat stuff - though I'm
>>>>>>>>>>>>> not as able to review in detail some of the refactoring of
>>>>>>>>>>>>> existing typo
>>>>>>>>>>>>> correction stuff. It looks plausible.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> I've attached the updated patch. Thanks for reviewing this,
>>>>>>>>>>>> David!
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>> On Thu, Sep 25, 2014 at 2:00 PM, Kaelyn Takata <
>>>>>>>>>>>>> [email protected]> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> ping
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Tue, Aug 26, 2014 at 11:05 AM, Kaelyn Takata <
>>>>>>>>>>>>>> [email protected]> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> +dblaikie
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Thu, Jul 31, 2014 at 1:20 PM, Kaelyn Takata <
>>>>>>>>>>>>>>> [email protected]> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Part of the infrastructure is a map from a TypoExpr to the
>>>>>>>>>>>>>>>> Sema-specific
>>>>>>>>>>>>>>>> state needed to correct it, along with helpers to ease
>>>>>>>>>>>>>>>> dealing with the
>>>>>>>>>>>>>>>> state.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> The the typo count is propagated up the stack of
>>>>>>>>>>>>>>>> ExpressionEvaluationContextRecords when one is popped off
>>>>>>>>>>>>>>>> of to
>>>>>>>>>>>>>>>> avoid accidentally dropping TypoExprs on the floor. For
>>>>>>>>>>>>>>>> example,
>>>>>>>>>>>>>>>> the attempted correction of g() in
>>>>>>>>>>>>>>>> test/CXX/class/class.mem/p5-0x.cpp
>>>>>>>>>>>>>>>> happens with an ExpressionEvaluationContextRecord that is
>>>>>>>>>>>>>>>> popped off
>>>>>>>>>>>>>>>> the stack prior to ActOnFinishFullExpr being called and the
>>>>>>>>>>>>>>>> tree
>>>>>>>>>>>>>>>> transform for TypoExprs being run.
>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>> include/clang/Sema/Sema.h | 44 +++++
>>>>>>>>>>>>>>>> include/clang/Sema/SemaInternal.h | 15 +-
>>>>>>>>>>>>>>>> include/clang/Sema/TypoCorrection.h | 2 +-
>>>>>>>>>>>>>>>> lib/Sema/SemaExpr.cpp | 7 +
>>>>>>>>>>>>>>>> lib/Sema/SemaExprCXX.cpp | 108 ++++++++++++
>>>>>>>>>>>>>>>> lib/Sema/SemaLookup.cpp | 316
>>>>>>>>>>>>>>>> ++++++++++++++++++++++++------------
>>>>>>>>>>>>>>>> 6 files changed, 384 insertions(+), 108 deletions(-)
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
