<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Dec 16, 2014 at 9:47 AM, Kaelyn Takata <span dir="ltr"><<a href="mailto:rikka@google.com" target="_blank">rikka@google.com</a>></span> wrote:<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">On Tue, Dec 16, 2014 at 12:13 AM, Nick Lewycky <span dir="ltr"><<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span>On 15 December 2014 at 18:12, Kaelyn Takata <span dir="ltr"><<a href="mailto:rikka@google.com" target="_blank">rikka@google.com</a>></span> wrote:<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">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:<div><br></div><div>1) The declaration of EmptyTypoCorrection in Sema::makeTypoCorrectionConsumer is unused,</div><div><br></div><div>2) The IsUnqualifiedLookup can be dropped from Sema::FailedCorrection now that it is unused within the function,</div><div><br></div><div>3) The EmptyTypoCorrection and ValidatingCallback variables in Sema::CorrectTypo can be dropped now that the third argument to Sema::FailedCorrection is unused, and<br></div><div><br></div><div>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).</div></div></blockquote><div><br></div></span><div>Done.</div><span><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>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.<br></div></div></blockquote><div><br></div></span><div>I wrote the code to remove the cache first then the flag as an add on afterwards. After hitting send I realized that I could split them apart and land the new flag first. Let me know if you need me to do that.</div><span><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div></div><div>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.<br></div></div></blockquote><div><br></div></span><div>Yes! That is certainly a bug, I think it's a bug in name lookup actually. That's a problem for another day.</div><div><br></div><div>I tried adding getCanonicalDecl(), but it breaks other cases. If the first declaration is a friend decl, then that one is now canonical and we will put the note on the friend decl instead of on the non-friend decl after it. For example:</div><div><br></div><div><div>error: 'note' diagnostics expected but not seen: </div><div> File test/CXX/class.access/class.friend/p1.cpp Line 39: 'S2' declared here</div><div> File test/CXX/class.access/class.friend/p1.cpp Line 39: 'S2' declared here</div><div> File test/CXX/class.access/class.friend/p1.cpp Line 40: 'g2' declared here</div><div> File test/CXX/class.access/class.friend/p1.cpp Line 40: 'g2' declared here</div><div>error: 'note' diagnostics seen but not expected: </div><div> File test/CXX/class.access/class.friend/p1.cpp Line 36: 'g2' declared here</div><div> File test/CXX/class.access/class.friend/p1.cpp Line 35: 'S2' declared here</div><div> File test/CXX/class.access/class.friend/p1.cpp Line 36: 'g2' declared here</div><div> File test/CXX/class.access/class.friend/p1.cpp Line 35: 'S2' declared here</div><div>8 errors generated.</div></div><div><br></div><div>I spent a while looking at the innards of name lookup to see what was wrong, and the issue is that LookupDirect returns anything that was passed in to makeDeclVisibleInContext before looking at other decls. Passing friend functions to makeDeclVisibleIn the outer Context is the right thing to do, it might be an ordering issue inside LookupQualifiedName or maybe we shouldn't stop looking just because we found one, or maybe findAcceptableDecl ought to do some more sorting instead of just filtering based on visibility.</div></div></div></div></blockquote><div><br></div></div></div><div>Ah, fair enough. Of course it can never be simple! I wonder if what's needed is a way to distinguish between a friend decl and a non-friend decl... or if we'd have to go so far as to flag decls created as a result of some diagnostic like typo correction so that they can be avoided when possible while generating notes for subsequent diagnostics. But like you said, it's a problem for another day; just stick a TODO or two in test/CXX/class.access/class.friend/p11.cpp as a guidepost and reminder that those notes shouldn't be referencing the typo-corrected friend decls.</div></div></div></div></blockquote><div><br></div><div>I think this should be pretty straightforward to fix: when name lookup finds a declaration whose IDNS contains both Ordinary/Tag and Friend, scan backwards over the redecl chain until you find one that isn't marked Friend nor LocalExtern (in MS compat mode, there might not be one, so you need to return the original one in that case). The only tricky part is picking the right place to insert this check into name lookup.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="HOEnZb"><font color="#888888"><div>- Kaelyn</div></font></span><span class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><font color="#888888"><div><br></div><div>Nick</div></font></span><span><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div><div class="gmail_extra"><div class="gmail_quote">On Sat, Dec 13, 2014 at 2:47 AM, Nick Lewycky <span dir="ltr"><<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>This patch removes Sema::UnqualifiedTyposCorrected. </div><div><br></div><div>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.</div><div><br></div><div>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?)</div>
<div><br></div><div>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. <br></div><div><br></div><div>Please review!</div><span><font color="#888888"><div><br></div><div>Nick</div><div><br></div></font></span></div>
</blockquote></div><br></div>
</div></div></blockquote></span></div></div></div>
</blockquote></span></div><br></div></div>
<br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div></div></div>