<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jan 20, 2015 at 4:32 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=richard@metafoo.co.uk&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Tue, Jan 20, 2015 at 4:27 PM, Kaelyn Takata <<a href="mailto:rikka@google.com" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=rikka@google.com&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">rikka@google.com</a>> wrote:<br>
><br>
><br>
> On Tue, Jan 20, 2015 at 2:46 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=richard@metafoo.co.uk&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">richard@metafoo.co.uk</a>><br>
> wrote:<br>
>><br>
>> On Tue, Jan 20, 2015 at 1:13 PM, Kaelyn Takata <<a href="mailto:rikka@google.com" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=rikka@google.com&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">rikka@google.com</a>> wrote:<br>
>> ><br>
>> ><br>
>> > On Wed, Jan 14, 2015 at 4:54 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=richard@metafoo.co.uk&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">richard@metafoo.co.uk</a>><br>
>> > wrote:<br>
>> >><br>
>> >> On Mon, Jan 5, 2015 at 2:47 PM, Kaelyn Takata <<a href="mailto:rikka@google.com" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=rikka@google.com&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">rikka@google.com</a>> wrote:<br>
>> >>><br>
>> >>> Just realized I accidently hit "Reply" instead of "Reply All" when<br>
>> >>> sending this email several weeks ago...<br>
>> >>><br>
>> >>> ---------- Forwarded message ----------<br>
>> >>> From: Kaelyn Takata <<a href="mailto:rikka@google.com" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=rikka@google.com&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">rikka@google.com</a>><br>
>> >>> Date: Sat, Dec 20, 2014 at 1:01 PM<br>
>> >>> Subject: Re: patch: remove the unqualified name cache for typos<br>
>> >>> To: Richard Smith <<a href="mailto:richard@metafoo.co.uk" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=richard@metafoo.co.uk&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">richard@metafoo.co.uk</a>><br>
>> >>><br>
>> >>><br>
>> >>><br>
>> >>><br>
>> >>> On Fri, Dec 19, 2014 at 5:00 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=richard@metafoo.co.uk&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">richard@metafoo.co.uk</a>><br>
>> >>> wrote:<br>
>> >>>><br>
>> >>>> On Fri, Dec 19, 2014 at 4:46 PM, Kaelyn Takata <<a href="mailto:rikka@google.com" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=rikka@google.com&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">rikka@google.com</a>><br>
>> >>>> wrote:<br>
>> >>>>><br>
>> >>>>> Here's a patch I whipped up that changes<br>
>> >>>>> NamedDecl::getUnderlyingDecl<br>
>> >>>>> to look back through friend decls to find a non-friend decl, which<br>
>> >>>>> causes<br>
>> >>>>> notes to be given in the right place in the test Nick changed<br>
>> >>>>> (test/CXX/class.access/class.friend/p11.cpp) and in another test<br>
>> >>>>> with FIXMEs<br>
>> >>>>> about a couple of notes being in the wrong place<br>
>> >>>>> (test/SemaTemplate/friend-template.cpp). Doing so seemed to be<br>
>> >>>>> appropriate<br>
>> >>>>> behavior for getUnderlyingDecl, instead of adding the code to look<br>
>> >>>>> back<br>
>> >>>>> through friend decls to both LookupResult::getFoundDecl and<br>
>> >>>>> TypoCorrection::addCorrectionDecl. This seem good?<br>
>> >>>><br>
>> >>>><br>
>> >>>> In your scan, you should also skip names in the LocalExtern<br>
>> >>>> namespace.<br>
>> >>>> In particular, given:<br>
>> >>>><br>
>> >>>> void f(int n);<br>
>> >>>> void g() { void f(int n = 0); }<br>
>> >>>> struct S { friend void f(int n); };<br>
>> >>>> void h() { f(); }<br>
>> >>>><br>
>> >>>> I think you'll skip backwards from line 3 to line 2, but we want to<br>
>> >>>> find<br>
>> >>>> the declaration on line 1. The end result will presumably be that we<br>
>> >>>> accept<br>
>> >>>> this ill-formed code.<br>
>> >>>><br>
>> >>>> I'm also not sure that getUnderlyingDecl is the right place to skip<br>
>> >>>> friends; my inclination is that this is something that should happen<br>
>> >>>> internally within name lookup (name lookup should act as if it simply<br>
>> >>>> does<br>
>> >>>> not find these friend declarations), rather than being something done<br>
>> >>>> by<br>
>> >>>> consumers of the lookup result.<br>
>> >>><br>
>> >>><br>
>> >>> I tried adding the logic to the various places where the Lookup*<br>
>> >>> functions/methods add decls to the LookupResult, and the primary place<br>
>> >>> that<br>
>> >>> needs it to make the two tests my patch changed work properly breaks<br>
>> >>> another<br>
>> >>> test in a way that I'm pretty sure is a real failure. The change in<br>
>> >>> question<br>
>> >>> is:<br>
>> >>><br>
>> >>> diff --git a/lib/Sema/SemaLookup.cpp b/lib/Sema/SemaLookup.cpp<br>
>> >>> index 3445264..9d85555 100644<br>
>> >>> --- a/lib/Sema/SemaLookup.cpp<br>
>> >>> +++ b/lib/Sema/SemaLookup.cpp<br>
>> >>> @@ -660,6 +660,13 @@ static void<br>
>> >>> DeclareImplicitMemberFunctionsWithName(Sema &S,<br>
>> >>> }<br>
>> >>> }<br>
>> >>><br>
>> >>> +static NamedDecl* getRealDecl(NamedDecl *ND) {<br>
>> >>> + Decl *Primary = ND;<br>
>> >>> + while (Primary &&<br>
>> >>> Primary->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend))<br>
>> >>> + Primary = Primary->getPreviousDecl();<br>
>> >>> + return Primary ? cast<NamedDecl>(Primary) : ND;<br>
>> >>> +}<br>
>> >>> +<br>
>> >>> // Adds all qualifying matches for a name within a decl context to<br>
>> >>> the<br>
>> >>> // given lookup result. Returns true if any matches were found.<br>
>> >>> static bool LookupDirect(Sema &S, LookupResult &R, const DeclContext<br>
>> >>> *DC) {<br>
>> >>> @@ -675,7 +682,7 @@ static bool LookupDirect(Sema &S, LookupResult &R,<br>
>> >>> const DeclContext *DC) {<br>
>> >>> ++I) {<br>
>> >>> NamedDecl *D = *I;<br>
>> >>> if ((D = R.getAcceptableDecl(D))) {<br>
>> >>> - R.addDecl(D);<br>
>> >>> + R.addDecl(getRealDecl(D));<br>
>> >>> Found = true;<br>
>> >>> }<br>
>> >>> }<br>
>> >>><br>
>> >>> And the test that legitimately starts failing is CXX/drs/dr1xx.cpp:<br>
>> >>><br>
>> >>> error: 'error' diagnostics expected but not seen:<br>
>> >>> File ~/llvm/tools/clang/test/CXX/drs/dr1xx.cpp Line 391: friend<br>
>> >>> declaration specifying a default argument must be the only declaration<br>
>> >>> error: 'error' diagnostics seen but not expected:<br>
>> >>> File ~/llvm/tools/clang/test/CXX/drs/dr1xx.cpp Line 391: missing<br>
>> >>> default argument on parameter<br>
>> >>> error: 'note' diagnostics expected but not seen:<br>
>> >>> File ~/llvm/tools/clang/test/CXX/drs/dr1xx.cpp Line 385: previous<br>
>> >>> declaration is here<br>
>> >>> 3 errors generated.<br>
>> >>><br>
>> >>> So the only two options I can see for getting the notes right is the<br>
>> >>> patch to getUnderlyingDecl that I sent earlier, or my original<br>
>> >>> approach<br>
>> >>> which was to add the logic for going back through the friend decls to<br>
>> >>> both<br>
>> >>> TransformTypos::EmitAllDiagnostics and LookupResult::getFoundDecl.<br>
>> >><br>
>> >><br>
>> >> I think there's one more tweak missing here: we should only skip over<br>
>> >> friend declarations if we're not performing redeclaration lookup.<br>
>> ><br>
>> ><br>
>> > I tried my last patch with a slight tweak to only call getRealDecl when<br>
>> > not<br>
>> > performing redeclaration lookup, and while it fixes the above error it<br>
>> > also<br>
>> > undoes the fix for the note locations. Changing the line with the call<br>
>> > to<br>
>> > getRealDecl to "R.addDecl(R.isForRedeclaration() ? D : getRealDecl(D));"<br>
>> > results in:<br>
>> ><br>
>> > error: 'note' diagnostics expected but not seen:<br>
>> > File ~/llvm/tools/clang/test/SemaTemplate/friend-template.cpp Line 72:<br>
>> > previous non-type template parameter with type 'int' is here<br>
>> > File ~/llvm/tools/clang/test/SemaTemplate/friend-template.cpp Line<br>
>> > 297:<br>
>> > previous non-type template parameter with type 'int' is here<br>
>> > error: 'note' diagnostics seen but not expected:<br>
>> > File ~/llvm/tools/clang/test/SemaTemplate/friend-template.cpp Line 78:<br>
>> > previous non-type template parameter with type 'int' is here<br>
>> > File ~/llvm/tools/clang/test/SemaTemplate/friend-template.cpp Line<br>
>> > 300:<br>
>> > previous non-type template parameter with type 'int' is here<br>
>> > 4 errors generated.<br>
>> ><br>
>> > Which puts the seen diagnostics back to what friend-template.cpp<br>
>> > currently<br>
>> > expects and has FIXMEs for. :(<br>
>><br>
>> I think that's a separate issue: I think that it's correct for<br>
>> redeclaration lookup to find the declaration on line 78 from the<br>
>> instantiation of X3<int>; that is the most recent preceding<br>
>> declaration of the same entity in the same scope (friend injection<br>
>> results in friends being declared in the innermost enclosing non-class<br>
>> scope).<br>
><br>
><br>
> I don't think that's quite right, at least in this case.... the friend<br>
> declaration on line 78 has the error "template non-type parameter has a<br>
> different type 'long' in template redeclaration" so it seems weird to point<br>
> the note from another error to a redeclaration that has an error in it.<br>
<br>
</div></div>That's not what's happening: the surrounding template is instantiated<br>
twice. The first time it produces<br>
<br>
template<typename U, int V> struct /*...*/<br>
<br>
and the second time it produces<br>
<br>
template<typename U, long V> struct /*...*/<br>
<br>
It's the mismatch between these two that we're diagnosing. Note that<br>
the first of these two does not have any errors.<br></blockquote><div><br></div><div>Ah, I see. In that case, I'll defer to you as to whether the FIXMEs are even valid (if they aren't valid, it's probably worth adding a note in their place briefly explaining why the notes are in the right place--that the errors on those decls are from a specific instantiation of the template decl, not the template decl itself).</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
> Same<br>
> for the friend declaration on line 300 vs the actual declaration on line<br>
> 297. Though it just occurred to me the solution would be to ignore the<br>
> friend declarations that had errors... but unfortunately they aren't marked<br>
> as invalid (changing getRealDecl to only get the previous decl when<br>
> isInvalidDecl is also true for the current decl didn't change which decls<br>
> the notes referred to).<br>
>><br>
>><br>
>><br>
>> >>>> That said, the test changes look like a real improvement.<br>
>> >>>><br>
>> >>>>> On Thu, Dec 18, 2014 at 5:34 PM, Richard Smith<br>
>> >>>>> <<a href="mailto:richard@metafoo.co.uk" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=richard@metafoo.co.uk&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">richard@metafoo.co.uk</a>><br>
>> >>>>> wrote:<br>
>> >>>>>><br>
>> >>>>>> On Tue, Dec 16, 2014 at 9:47 AM, Kaelyn Takata <<a href="mailto:rikka@google.com" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=rikka@google.com&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">rikka@google.com</a>><br>
>> >>>>>> wrote:<br>
>> >>>>>>><br>
>> >>>>>>><br>
>> >>>>>>><br>
>> >>>>>>> On Tue, Dec 16, 2014 at 12:13 AM, Nick Lewycky<br>
>> >>>>>>> <<a href="mailto:nlewycky@google.com" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=nlewycky@google.com&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">nlewycky@google.com</a>><br>
>> >>>>>>> wrote:<br>
>> >>>>>>>><br>
>> >>>>>>>> On 15 December 2014 at 18:12, Kaelyn Takata <<a href="mailto:rikka@google.com" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=rikka@google.com&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">rikka@google.com</a>><br>
>> >>>>>>>> wrote:<br>
>> >>>>>>>>><br>
>> >>>>>>>>> In general this patch looks good. And it is probably time to<br>
>> >>>>>>>>> kill<br>
>> >>>>>>>>> the cache anyway now that so much of the typo correction<br>
>> >>>>>>>>> bypasses it anyway<br>
>> >>>>>>>>> (correction callbacks that implement a meaningful<br>
>> >>>>>>>>> ValidateCandidate, and the<br>
>> >>>>>>>>> delayed typo correction for the most part). A few things that<br>
>> >>>>>>>>> should be<br>
>> >>>>>>>>> cleaned up though:<br>
>> >>>>>>>>><br>
>> >>>>>>>>> 1) The declaration of EmptyTypoCorrection in<br>
>> >>>>>>>>> Sema::makeTypoCorrectionConsumer is unused,<br>
>> >>>>>>>>><br>
>> >>>>>>>>> 2) The IsUnqualifiedLookup can be dropped from<br>
>> >>>>>>>>> Sema::FailedCorrection now that it is unused within the<br>
>> >>>>>>>>> function,<br>
>> >>>>>>>>><br>
>> >>>>>>>>> 3) The EmptyTypoCorrection and ValidatingCallback variables in<br>
>> >>>>>>>>> Sema::CorrectTypo can be dropped now that the third argument to<br>
>> >>>>>>>>> Sema::FailedCorrection is unused, and<br>
>> >>>>>>>>><br>
>> >>>>>>>>> 4) IsUnqualifiedLookup can be made a local variable within<br>
>> >>>>>>>>> Sema::makeTypoCorrectionConsumer instead of a out-parameter and<br>
>> >>>>>>>>> the<br>
>> >>>>>>>>> corresponding variables in CorrectTypo and CorrectTypoDelayed<br>
>> >>>>>>>>> can be<br>
>> >>>>>>>>> removed, now that CorrectTypo doesn't need to know whether the<br>
>> >>>>>>>>> lookup is<br>
>> >>>>>>>>> qualified or not (which was simply passed along to various calls<br>
>> >>>>>>>>> to<br>
>> >>>>>>>>> FailedCorrection).<br>
>> >>>>>>>><br>
>> >>>>>>>><br>
>> >>>>>>>> Done.<br>
>> >>>>>>>><br>
>> >>>>>>>>><br>
>> >>>>>>>>> However, as this patch is already fairly large (removing the<br>
>> >>>>>>>>> cache,<br>
>> >>>>>>>>> adding the new flag, and making use of it to reunify the two<br>
>> >>>>>>>>> main typo<br>
>> >>>>>>>>> correction test files) I'm fine with 2-4 being done in a<br>
>> >>>>>>>>> separate cleanup<br>
>> >>>>>>>>> commit.<br>
>> >>>>>>>><br>
>> >>>>>>>><br>
>> >>>>>>>> I wrote the code to remove the cache first then the flag as an<br>
>> >>>>>>>> add<br>
>> >>>>>>>> on afterwards. After hitting send I realized that I could split<br>
>> >>>>>>>> them apart<br>
>> >>>>>>>> and land the new flag first. Let me know if you need me to do<br>
>> >>>>>>>> that.<br>
>> >>>>>>>><br>
>> >>>>>>>>> The only concern I have with this patch is actually incidental<br>
>> >>>>>>>>> to<br>
>> >>>>>>>>> it: in test/CXX/class.access/class.friend/p11.cpp that the notes<br>
>> >>>>>>>>> from the<br>
>> >>>>>>>>> newly exposed suggestions are wrong--they are pointing to<br>
>> >>>>>>>>> previous friend<br>
>> >>>>>>>>> declarations (which had also been typo-corrected) instead of to<br>
>> >>>>>>>>> the actual<br>
>> >>>>>>>>> declarations of ::test2::foo and ::test2::bar. I suspect this is<br>
>> >>>>>>>>> a<br>
>> >>>>>>>>> relatively simple fix, probably a matter using the the<br>
>> >>>>>>>>> primary/canonical/first delcaration instead of simply the most<br>
>> >>>>>>>>> recent one<br>
>> >>>>>>>>> for the NamedDecl descendants that are also Redeclarable (such<br>
>> >>>>>>>>> as<br>
>> >>>>>>>>> FunctionDecl and RecordDecl), though it is also an important one<br>
>> >>>>>>>>> from a QoI<br>
>> >>>>>>>>> perspective as that test demonstrates the "XYZ declared here"<br>
>> >>>>>>>>> pointing to<br>
>> >>>>>>>>> incorrect and potentially confusing locations.<br>
>> >>>>>>>><br>
>> >>>>>>>><br>
>> >>>>>>>> Yes! That is certainly a bug, I think it's a bug in name lookup<br>
>> >>>>>>>> actually. That's a problem for another day.<br>
>> >>>>>>>><br>
>> >>>>>>>> I tried adding getCanonicalDecl(), but it breaks other cases. If<br>
>> >>>>>>>> the<br>
>> >>>>>>>> first declaration is a friend decl, then that one is now<br>
>> >>>>>>>> canonical and we<br>
>> >>>>>>>> will put the note on the friend decl instead of on the non-friend<br>
>> >>>>>>>> decl after<br>
>> >>>>>>>> it. For example:<br>
>> >>>>>>>><br>
>> >>>>>>>> error: 'note' diagnostics expected but not seen:<br>
>> >>>>>>>> File test/CXX/class.access/class.friend/p1.cpp Line 39: 'S2'<br>
>> >>>>>>>> declared here<br>
>> >>>>>>>> File test/CXX/class.access/class.friend/p1.cpp Line 39: 'S2'<br>
>> >>>>>>>> declared here<br>
>> >>>>>>>> File test/CXX/class.access/class.friend/p1.cpp Line 40: 'g2'<br>
>> >>>>>>>> declared here<br>
>> >>>>>>>> File test/CXX/class.access/class.friend/p1.cpp Line 40: 'g2'<br>
>> >>>>>>>> declared here<br>
>> >>>>>>>> error: 'note' diagnostics seen but not expected:<br>
>> >>>>>>>> File test/CXX/class.access/class.friend/p1.cpp Line 36: 'g2'<br>
>> >>>>>>>> declared here<br>
>> >>>>>>>> File test/CXX/class.access/class.friend/p1.cpp Line 35: 'S2'<br>
>> >>>>>>>> declared here<br>
>> >>>>>>>> File test/CXX/class.access/class.friend/p1.cpp Line 36: 'g2'<br>
>> >>>>>>>> declared here<br>
>> >>>>>>>> File test/CXX/class.access/class.friend/p1.cpp Line 35: 'S2'<br>
>> >>>>>>>> declared here<br>
>> >>>>>>>> 8 errors generated.<br>
>> >>>>>>>><br>
>> >>>>>>>> I spent a while looking at the innards of name lookup to see what<br>
>> >>>>>>>> was wrong, and the issue is that LookupDirect returns anything<br>
>> >>>>>>>> that was<br>
>> >>>>>>>> passed in to makeDeclVisibleInContext before looking at other<br>
>> >>>>>>>> decls. Passing<br>
>> >>>>>>>> friend functions to makeDeclVisibleIn the outer Context is the<br>
>> >>>>>>>> right thing<br>
>> >>>>>>>> to do, it might be an ordering issue inside LookupQualifiedName<br>
>> >>>>>>>> or maybe we<br>
>> >>>>>>>> shouldn't stop looking just because we found one, or maybe<br>
>> >>>>>>>> findAcceptableDecl ought to do some more sorting instead of just<br>
>> >>>>>>>> filtering<br>
>> >>>>>>>> based on visibility.<br>
>> >>>>>>><br>
>> >>>>>>><br>
>> >>>>>>> Ah, fair enough. Of course it can never be simple! I wonder if<br>
>> >>>>>>> what's<br>
>> >>>>>>> needed is a way to distinguish between a friend decl and a<br>
>> >>>>>>> non-friend<br>
>> >>>>>>> decl... or if we'd have to go so far as to flag decls created as a<br>
>> >>>>>>> result of<br>
>> >>>>>>> some diagnostic like typo correction so that they can be avoided<br>
>> >>>>>>> when<br>
>> >>>>>>> possible while generating notes for subsequent diagnostics. But<br>
>> >>>>>>> like you<br>
>> >>>>>>> said, it's a problem for another day; just stick a TODO or two in<br>
>> >>>>>>> test/CXX/class.access/class.friend/p11.cpp as a guidepost and<br>
>> >>>>>>> reminder that<br>
>> >>>>>>> those notes shouldn't be referencing the typo-corrected friend<br>
>> >>>>>>> decls.<br>
>> >>>>>><br>
>> >>>>>><br>
>> >>>>>> I think this should be pretty straightforward to fix: when name<br>
>> >>>>>> lookup<br>
>> >>>>>> finds a declaration whose IDNS contains both Ordinary/Tag and<br>
>> >>>>>> Friend, scan<br>
>> >>>>>> backwards over the redecl chain until you find one that isn't<br>
>> >>>>>> marked Friend<br>
>> >>>>>> nor LocalExtern (in MS compat mode, there might not be one, so you<br>
>> >>>>>> need to<br>
>> >>>>>> return the original one in that case). The only tricky part is<br>
>> >>>>>> picking the<br>
>> >>>>>> right place to insert this check into name lookup.<br>
>> >>>>>><br>
>> >>>>>>> - Kaelyn<br>
>> >>>>>>>><br>
>> >>>>>>>><br>
>> >>>>>>>> Nick<br>
>> >>>>>>>><br>
>> >>>>>>>>> On Sat, Dec 13, 2014 at 2:47 AM, Nick Lewycky<br>
>> >>>>>>>>> <<a href="mailto:nlewycky@google.com" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=nlewycky@google.com&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">nlewycky@google.com</a>><br>
>> >>>>>>>>> wrote:<br>
>> >>>>>>>>>><br>
>> >>>>>>>>>> This patch removes Sema::UnqualifiedTyposCorrected.<br>
>> >>>>>>>>>><br>
>> >>>>>>>>>> I'm not sure this is the right thing to do. As far as I can<br>
>> >>>>>>>>>> tell<br>
>> >>>>>>>>>> this cache can't be made to be correct (wouldn't we need to<br>
>> >>>>>>>>>> invalidate it<br>
>> >>>>>>>>>> every time a new decl is declared?) and I'm not convinced it<br>
>> >>>>>>>>>> saves us much<br>
>> >>>>>>>>>> compile time. However, I'm not confident in my understanding of<br>
>> >>>>>>>>>> the code, so<br>
>> >>>>>>>>>> please do push back if it is beneficial.<br>
>> >>>>>>>>>><br>
>> >>>>>>>>>> One bug it fixes is a case where the same full-expression has<br>
>> >>>>>>>>>> the<br>
>> >>>>>>>>>> two typos in the same identifier, and we emit the correction at<br>
>> >>>>>>>>>> the<br>
>> >>>>>>>>>> SourceRange for the first typo twice instead of once for each<br>
>> >>>>>>>>>> place it's<br>
>> >>>>>>>>>> misspelled. This is because the SourceRange is on the<br>
>> >>>>>>>>>> TypoCorrection, and<br>
>> >>>>>>>>>> the cache key is an IdentifierInfo. Adding the SourceRange to<br>
>> >>>>>>>>>> the cache key<br>
>> >>>>>>>>>> wouldn't make much sense because we wouldn't ever re-use it.<br>
>> >>>>>>>>>> (NB, I have<br>
>> >>>>>>>>>> similar feelings about TypoCorrectionFailures which I am not<br>
>> >>>>>>>>>> touching in<br>
>> >>>>>>>>>> this patch. Why would we do typo correction twice at the same<br>
>> >>>>>>>>>> location?)<br>
>> >>>>>>>>>><br>
>> >>>>>>>>>> Removing it improves typo correction quality and there are<br>
>> >>>>>>>>>> changes<br>
>> >>>>>>>>>> to the tests (see p11.cpp, and the last two tests in<br>
>> >>>>>>>>>> typo-correction) where<br>
>> >>>>>>>>>> it fired. Because the cutoff for typos is a function of this<br>
>> >>>>>>>>>> cache, I move<br>
>> >>>>>>>>>> that to a flag (-fspell-checking-limit=...) and turn the limit<br>
>> >>>>>>>>>> off entirely<br>
>> >>>>>>>>>> to merge typo-correction-pt2.cpp and typo-correction.cpp. Also<br>
>> >>>>>>>>>> turn it up<br>
>> >>>>>>>>>> from 20 to 50 to accommodate existing tests. Without caching,<br>
>> >>>>>>>>>> we run up the<br>
>> >>>>>>>>>> counter more quickly.<br>
>> >>>>>>>>>><br>
>> >>>>>>>>>> Please review!<br>
>> >>>>>>>>>><br>
>> >>>>>>>>>> Nick<br>
>> >>>>>>>>>><br>
>> >>>>>>>>><br>
>> >>>>>>><br>
>> >>>>>>><br>
>> >>>>>>> _______________________________________________<br>
>> >>>>>>> cfe-commits mailing list<br>
>> >>>>>>> <a href="mailto:cfe-commits@cs.uiuc.edu" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=cfe-commits@cs.uiuc.edu&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">cfe-commits@cs.uiuc.edu</a><br>
>> >>>>>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
>> >>>>>>><br>
>> >>><br>
>> >><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>