Hello Doug,<br><br><div class="gmail_quote">On Wed, Jun 1, 2011 at 9:56 AM, Douglas Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com">dgregor@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div style="word-wrap:break-word">Hello Kaelyn,<div><br></div><div>Sorry for the sloooow response.</div></div></blockquote><div><br>It's no problem. After all, life has a nasty habit of getting in the way of coding. ;)<br>
<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;"><div><br><div><div class="im"><div>On May 20, 2011, at 4:17 PM, Kaelyn Uhrain wrote:</div>
<br><blockquote type="cite">Here's a WIP version of my rewritten namespace-aware typo correction patch based on your feedback and our previous discussion. Right now there are two versions of Sema::CorrectTypo, with one for compatibility with the original version of the method, and only lib/Sema/SemaExpr.cpp has been modified to use the new one directly. With just SemaExpr.cpp hooked up, the number of errors in the new unit test dropped from 58 to 48, and currently the only failure in existing tests which I still need to iron out is test/CXX/basic/basic.lookup/basic.lookup.argdep/p4.cpp (I expect there will be more in C++ tests as new corrections become possible).<br>
</blockquote><div><br></div></div><div>Sure, that makes sense. I'm fine with bringing in a new version of CorrectTypo, and then switching over call sites one-by-one until we can kill the old version. </div></div></div>
</div></blockquote><div><br>I intended it as a temporary measure while working on the patch so that I wouldn't have to touch all of the call sites as I tweaked and honed the interface of the new CorrectTypo, but thinking about it a bit more, fixing up the call sites as separate commit(s) would be useful for managing changes in behavior, tracking down regressions or unintended consequences, etc.<br>
<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;"><div><div><div class="im"><blockquote type="cite">
I still have a fair bit of work to do such as having the rest of the callers of CorrectTypo look for correction results in the new TypoCorrection object instead of LookupResults, and to see about streamlining Sema::CorrectTypo a bit more. I'm sending what I have so far not for submission but to get your feedback on the new approach before I put a lot of effort into polishing it.<br>
</blockquote><div><br></div></div><div>A few comments:</div><div><br></div><div><div>+ // \brief The set of known/encountered (unique, canonicalized) NamespaceDecls</div><div>+ llvm::SmallPtrSet<NamespaceDecl*, 16> KnownNamespaces;</div>
<div><br></div><div>This is going to need to get serialized to the AST/PCH file, so that typo correction for qualified names works for namespaces defined in headers that go into the precompiled header.</div></div></div></div>
</div></blockquote><div><br>I might need a few pointers on this as I've never really used PCH files, much less touched any of the clang code that reads/writes them...<br> <br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div style="word-wrap: break-word;"><div><div><div><div><br></div></div><div><div>+ } else if (!Namespc->isInline()) { // TODO(rikka): Do inline namespaces need to be scanned?</div><div>+ // Since this is an "original" namespace, add it to the known set of</div>
<div>+ // namespaces if it is not an inline namespace.</div><div>+ KnownNamespaces.insert(Namespc);</div><div><br></div><div>Members of an inline namespace will be found via name lookup in their enclosing namespace, so we don't need to record inline namespaces for the purposes of spell-checking.</div>
</div></div></div></div></blockquote><div><br>Okay, thanks. I thought that was the case but stuck that TODO in to make sure I verified the behavior before "officially" submitting this patch, since my previous version didn't handle inline namespaces correctly.<br>
<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;"><div><div><div><br></div><div>@@ -2807,7 +2808,13 @@ LabelDecl *Sema::LookupOrCreateLabel(IdentifierInfo *II, SourceLocation Loc,</div>
<div> // Typo correction</div><div> //===----------------------------------------------------------------------===//</div><div> </div><div>+#define MAX_BUCKETS 5</div><div><br></div><div>Could we get a more descriptive name than MAX_BUCKETS? And can it be a "const unsigned" rather than a #define?</div>
</div></div></div></blockquote><div><br>Changed to "static const unsigned MaxTypoDistanceResultSets = 5;"<br><br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div style="word-wrap: break-word;"><div><div><div><br></div><div><div>+void TypoCorrectionConsumer::addName(llvm::StringRef Name,</div><div>+ NamedDecl *ND,</div><div>+ unsigned Distance,</div>
<div>+ NestedNameSpecifier *NNS) {</div><div>+ BestResults[Distance][Name] = TypoCorrection(&SemaRef.Context.Idents.get(Name),</div><div>+ ND, NNS);</div>
<div>+</div><div>+ while (BestResults.size() > MAX_BUCKETS) {</div><div>+ BestResults.erase(BestResults.rbegin()->first);</div><div>+ BestEditDistance = BestResults.rbegin()->first;</div><div>+ }</div><div>
}</div><div><br></div><div>How about using</div><div><br></div><div><span style="white-space:pre-wrap"> </span><span style="color:rgb(80, 129, 135);font-family:Menlo;font-size:11px">BestResults</span><span style="color:rgb(80, 129, 135);font-family:Menlo;font-size:11px"><span style="color:#000000">.</span></span><span style="color:rgb(80, 129, 135);font-family:Menlo;font-size:11px"><span style="color:#3c2480">erase</span></span><span style="color:rgb(80, 129, 135);font-family:Menlo;font-size:11px"><span style="color:#000000">(--</span></span><span style="color:rgb(80, 129, 135);font-family:Menlo;font-size:11px">BestResults</span><span style="color:rgb(80, 129, 135);font-family:Menlo;font-size:11px"><span style="color:#000000">.</span></span><span style="color:rgb(80, 129, 135);font-family:Menlo;font-size:11px"><span style="color:#3c2480">end</span></span><span style="color:rgb(80, 129, 135);font-family:Menlo;font-size:11px"><span style="color:#000000">());</span></span></div>
<div><span style="color:rgb(80, 129, 135);font-family:Menlo;font-size:11px"><span style="color:#000000"><br></span></span></div><div><span style="color:rgb(80, 129, 135);font-family:Menlo;font-size:11px"><span style="color:#000000">?</span></span></div>
</div></div></div></div></blockquote><div><br>Wasn't sure if decrementing the end() iterator would work. Changed!<br> <br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div style="word-wrap: break-word;"><div><div><div><div><span style="color:rgb(80, 129, 135);font-family:Menlo;font-size:11px"><span style="color:#000000"><br></span></span></div></div><div><div>+/// \brief Add keywords to the consumer. Moved out of Sema::CorrectTypo as it</div>
<div>+/// contains little logic related to typo correction and was 170+ lines of "add</div><div>+/// this and this and this" in the middle of the typo correction logic.</div><div>+static void AddKeywordsToConsumer(Sema &SemaRef,</div>
<div>+ TypoCorrectionConsumer &Consumer,</div><div>+ Scope *S, Sema::CorrectTypoContext CTC) {</div><div><br></div><div>Thanks for moving this; the comment doesn't need to reflect that this was refactored (that information goes into the commit message).</div>
</div></div></div></div></blockquote><div><br>You're welcome. And I removed my rambling from the comment. ;)<br> <br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div style="word-wrap: break-word;"><div><div><div><div><br></div></div><div><div>+/// \brief Helper for building the list of DeclContexts between the current</div><div>+/// context and the top of the translation unit</div>
<div>+typedef std::deque<DeclContext*> DeclContextList;</div><div>+static DeclContextList BuildContextChain(DeclContext *Start) {</div><div>+ DeclContextList Chain;</div><div>+ for (DeclContext *DC = Start; DC != NULL; DC = DC->getLookupParent()) {</div>
<div>+ if (!DC->isInlineNamespace() && !DC->isTransparentContext())</div><div>+ Chain.push_front(DC->getPrimaryContext());</div><div>+ }</div><div>+ return Chain;</div><div>+}</div><div><br></div>
<div>I think you also want to skip anonymous namespaces here. It also makes sense to always work with the primary context (e.g., starting from Start->getPrimaryContext(), so that we don't get tripped up by parallel-structured redefinitions:</div>
<div><br></div><div><span style="white-space:pre-wrap"> </span>namespace std { namespace rel_ops { } }</div><div><div><span style="white-space:pre-wrap"> </span>namespace std { namespace rel_ops { } }</div></div></div></div>
</div></div></blockquote><div><br>Thanks for catching that! Made the change to Start. Adding the check for anonymous namespaces was a bit messier since DeclContext doesn't expose that check.<br><br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div style="word-wrap: break-word;"><div><div><div><div><br></div><div>Also, a deque seems really heavyweight for this, since the typical length of this chain will be < 2. How about using an llvm::SmallVector and reverse()'ing it after construction?</div>
</div></div></div></div></blockquote><div><br>Hmm... good point. Working on switching over to an llvm::SmallVector.<br><br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div style="word-wrap: break-word;"><div><div><div><div><br></div><div><div>+ // Only perform the qualified lookups for C++</div><div>+ // FIXME: this breaks test/CXX/basic/basic.lookup/basic.lookup.argdep/p4.cpp</div>
<div>+ if (getLangOptions().CPlusPlus) {</div><div>+ TmpRes.suppressDiagnostics();</div><div>+ for (llvm::SmallPtrSet<IdentifierInfo*,</div><div>+ 16>::iterator QRI = QualifiedResults.begin(),</div>
<div>+ QRIEnd = QualifiedResults.end();</div><div>+ QRI != QRIEnd; ++QRI) {</div><div>+ for (llvm::SmallPtrSet<NamespaceDecl*,</div><div>+ 16>::iterator KNI = KnownNamespaces.begin(),</div>
<div>+ KNIEnd = KnownNamespaces.end();</div><div>+ KNI != KNIEnd; ++KNI) {</div><div>+ NameSpecifierAndSize NSS;</div><div>+ DeclContext *Ctx = dyn_cast<DeclContext>(*KNI);</div>
<div>+ TmpRes.clear();</div><div>+ TmpRes.setLookupName(*QRI);</div><div>+ if (!LookupQualifiedName(TmpRes, Ctx)) continue;</div><div>+</div><div>+ switch (TmpRes.getResultKind()) {</div>
<div>+ case LookupResult::Found:</div><div>+ case LookupResult::FoundOverloaded:</div><div>+ case LookupResult::FoundUnresolvedValue:</div><div>+ if (SpecifierMap.find(Ctx) != SpecifierMap.end()) {</div>
<div>+ NSS = SpecifierMap[Ctx];</div><div>+ } else {</div><div>+ NSS = BuildSpecifier(Context, CurContextChain, Ctx);</div><div>+ SpecifierMap[Ctx] = NSS;</div><div>+ }</div>
<div>+ Consumer.addName((*QRI)->getName(), TmpRes.getAsSingle<NamedDecl>(),</div><div>+ ED + NSS.second, NSS.first);</div><div>+ break;</div><div>+ case LookupResult::NotFound:</div>
<div>+ case LookupResult::NotFoundInCurrentInstantiation:</div><div>+ case LookupResult::Ambiguous:</div><div>+ break;</div><div>+ }</div><div>+ }</div><div> }</div><div>
}</div></div><div><br></div><div>I suggest always calling TmpRes.suppressDiagnostics() in the ambiguous case, otherwise you may end up getting ambiguity warnings. I don't know if that's the p4.cpp issue or not.</div>
</div></div></div></div></blockquote><div><br>Its not necessary since TmpRes.suppressDiagnostics() was called just before the beginning of the outer loop. And to make sure nothing sneaky is happening, I added an assertion that diagnostics are still suppressed to the ambiguous case, and at least for the test suite it was never triggered. I think the p4.cpp issue is related to the typo correction not (yet) being smart enough regarding argument dependent lookup.<br>
<br> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;"><div><div><div><div><br></div><div>I suspect that we'll want to order the KnownNamespaces traversal from shortest NNS to longest NNS, so we can avoid performing name lookups (which are very expensive in the PCH case) when the NNS length + edit distance are worse than the best.</div>
</div></div></div></div></blockquote><div><br>Adding that heuristic...<br> <br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;">
<div><div><div><div><br></div><div>Also, a general question… when we return a typo-correction result that produces a qualified name, shouldn't we be updating the CXXScopeSpec with the nested-name-specifier we produce?</div>
</div></div></div></div></blockquote><div><br>IIRC it's not uncommon (at least with test/SemaCXX/missing-namespace-qualifier-typo-corrections.cpp) for CorrectTypo to be called with a NULL CXXScopeSpec pointer. Changing it to always be something we can update is one of those function-signature changes that affect the behavior of the call sites, which is the area I've done the least mucking with so far. It's on the to-do list as even the one place where I've changed the code to use the new version of CorrectTypo breaks if its local LookupResult isn't updated correctly with the outcome of calling CorrectTypo. Which is one of the bigger reasons why I have the compatibility wrapper.<br>
<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;"><div><div><div class="im"><blockquote type="cite">
I also have a couple of questions regarding the unqualified typo correction. The current behavior is that if the same typo shows up again, one of two things happen:<br> - if the cached value indicated a correction wasn't found last time then the same is returned this time<br>
- otherwise CorrectTypo checks for the best match using the cached identifier plus all possible keywords in the current context (and it would be theoretically possible to have the same typo yield a different correction if the typo is a closer match to a now-valid keyword than to the non-keyword identifier that had been returned last time).<br>
Do we want to stick to this semi-convoluted behavior, or shall I simplify the caching to always return the cached correction as long as it is still in scope or is a null match?</blockquote><div><br></div></div><div>I'm fine with changing this behavior to (almost) always use the cached correction, with two caveats:</div>
<div><span style="white-space:pre-wrap"> </span>(1) If a keyword is selected but isn't available in the current context (e.g., if we pick "for" but aren't in a statement context), we need to make sure we return "no result".</div>
<div><br></div><div><span style="white-space:pre-wrap"> </span>(2) We still need to do extra checks when dealing with context-sensitive keywords. You don't have to worry about this, because such context-sensitive keywords only matter for typo correction in Objective-C</div>
</div></div></div></blockquote><div><br>Sounds good. Thanks for the input.<br> <br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;">
<div><div><div class="im"><br><blockquote type="cite"> And with the new probing into namespaces, do we want to always retry the search when the cache indicates a null match (or possibly for a subset of instances, such as if the current DeclContext is different)?<br>
</blockquote><div><br></div></div><div>We could retry if the set of known namespaces has changed.</div></div></div></div></blockquote><div><br>Though this wouldn't quite cover all of the cases where a retry should be done, namely the case where an existing namespace has been extended since the null match was encountered.<br>
<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;"><div><div><div><br></div><div>This is looking great, and we're getting close to the point where I'd like to put it into the tree! The major issues that I think need to be resolved before we can commit are:</div>
<div><span style="white-space:pre-wrap"> </span>- Getting the same behavior when using PCH and when not using PCH (the serialization issue mentioned at the top) <br></div></div></div></div></blockquote><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div style="word-wrap: break-word;"><div><div><div><span style="white-space:pre-wrap"> </span>- Testsuite failures</div></div></div></div></blockquote><div><br>As I said, the PCH matter requires a bit more learning and experimenting on my part since I've not done anything with them before. Fixing the test suite failures are a given for me, and have been a requirement for submitting the patch for inclusion into the tree. ;)<br>
<br>Thanks again for your help and your great feedback, Doug!<br><br>Cheers,<br>Kaelyn<br><br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div style="word-wrap: break-word;"><div><div><div><br></div><div>Thanks, Kaelyn!</div><div><br></div><div><span style="white-space:pre-wrap"> </span>- Doug</div><div><br></div><blockquote type="cite"><div><div></div><div class="h5">
<div class="gmail_quote">On Sat, Mar 26, 2011 at 4:51 AM, Douglas Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com" target="_blank">dgregor@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><br><div><div><div>On Mar 25, 2011, at 10:15 PM, Kaelyn Uhrain wrote:</div><br><blockquote type="cite"><br><br><div class="gmail_quote">On Mon, Mar 14, 2011 at 10:28 AM, Douglas Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com" target="_blank">dgregor@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204, 204, 204);padding-left:1ex">
<div><br> 1) Don't walk the namespaces at all; let the check against all of the identifiers in the translation unit winnow the list down to the best candidates based on name alone. We don't know which of these candidates is visible, or how to get there.<br>
</div></blockquote><div><br>The problem with first checking by name alone is that you can mistakenly discard the best match--I had that problem in early iterations of my patch. Take for example:<br><br>namespace foo {<span><br>
namespace bar {<br> void tree_branches() </span> { /* ... */ }<span><br> }<br>}<br><br>void </span><span>three_branches() </span> { /* ... */ }<span><br><br>int main() {<br> tree_branches();<br>}<br><br>Currently my patch will suggest three_branches as it has an edit distance of 1 versus an edit distance of 2 for foo::bar::tree_branches. Checking against all identifiers first would cause the TypoCorrectionConsumer to discard three_branches in favor of tree_branches since it would think tree_branches has an edit distance of 0... but will later realize tree_branches is unreachable from the current context and discard it (as the code would do now), or possibly suggest a sub-optimal correction (if the code waited to compute the qualifier). So the trick would be to figure out the qualifier needed for each identifier before making a judgment about edit distance without doing an unqualified lookup and potentially trying to figure out a valid qualified lookup for each identifier. The trick is trying to get the right info at the right time without walking through the namespaces in the AST given that the IdentifierInfo objects stored in Context.Idents lack any reference to a declaration context.<br>
</span></div></div></blockquote><div><br></div></div>Okay, that makes sense. Since we have to go through all of the identifiers anyway, we could certainly just keep several different buckets---best edit distance, best edit distance-1, best edit distance-2---so we can catch these cases. Then, if nothing in the first bucket matches, we go on to the next-best bucket, and so on.</div>
<div><div><br><blockquote type="cite"><div class="gmail_quote"><div><span>
</span></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204, 204, 204);padding-left:1ex"><div></div>
2) When we're performing lookup for each candidate (e.g., the calls to LookupPotentialTypoResult), first try unqualified lookup. If that fails, save the candidate in a separate "try qualified name lookup" set.<br>
</blockquote><div><br>With my above comments in mind, the unqualified lookup would have to be done for potentially every identifier in the translation unit (any subsequent identifier that has a greater edit distance than the closest correction so far that was successfully lookup can be skipped, but the worst-case scenario is for all identifiers to need to be looked up), followed by qualifier computation (involving walking the decls in the AST for child namespaces, unless I'm missing some handy method for building qualified identifiers) on every identifier that failed the unqualified lookup but does not have a greater edit distance than the closest valid unqualified identifier.<br>
</div></div></blockquote><div><br></div></div><div>We can't look up all of the identifiers. It's much, much too expensive; that's why we have the system we have, so that we can quickly narrow down to a small set of entities that we perform lookups on.</div>
<div><br></div><div>It's better to guarantee acceptable performance and miss a few typo correction suggestions than it is to slow compilation down to a crawl due to, e.g., a missing header inclusion. </div><div>
<br><blockquote type="cite"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204, 204, 204);padding-left:1ex">
3) If none of the candidates can be found by unqualified lookup, start looking in each of the namespaces we know about to see if qualified name lookup would work to find each of the candidates. You'll likely need a side structure containing all of the (unique, canonicalized) NamespaceDecls in the translation unit to make this efficient.<br>
</blockquote><div><br>How would building the side structure containing all of those NamespaceDecls be more efficient than ScanNamespaceForCorrections(), which stops walking through namespaces once the distance between a namespace and the current context is greater than the best edit distance known to the TypoCorrectionConsumer? Or walk any less of the AST than ScanNamespaceForCorrections?<br>
</div></div></blockquote><div><br></div></div><div>If we're only looking up a small number of identifiers, then we can perform targeted lookups of those identifiers, which (in the PCH case) boils down to a fairly quick search in an on-disk hash table. Since the number of namespaces is generally much fewer than the number of total declarations, we're doing less work.</div>
<div><br><blockquote type="cite"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204, 204, 204);padding-left:1ex">
<br>
Implemented this way, there's no additional penalty for typo correction of visible names. Moreover, we skip the AST walk entirely, and only perform targeted lookups for names that we know exist (because they were in the identifier table). This also permits a few more optimizations:<br>
<br>
- We could (roughly) order namespaces based on their depth, so that we'll tend to visit the more top-level (and, therefore, more likely to have short qualifiers) namespaces first. If the qualifier for a namespace would be longer than the shortest qualifier we've found so far, we don't have to look in that namespace.<br>
</blockquote><div><br> ScanNamespaceForCorrections already skips namespaces that would require overly-long qualifiers.<br></div></div></blockquote><div><br></div></div><div>Sure, but in my experience, most large namespaces (std, boost, llvm, clang) are top-level namespaces, so pruning based on overly-long qualifiers is likely to only prune away the small namespaces. </div>
<div><br></div><div>I still advocate doing that pruning, but I want quick hash-table lookups based on the candidate identifiers rather than a weak of all of the contents of these namespaces.</div><div><div><br>
</div><blockquote type="cite"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204, 204, 204);padding-left:1ex">
- For a given namespace, we could look for all of the candidates in that namespace at the same time, improving locality (which is particularly import when those lookups hit the disk in the PCH case).<br></blockquote>
<div><br>ScanNamespaceForCorrections currently feeds candidates to the TypoCorrectionConsumer by namespace. Admittedly, it could store child NamespaceDecls to be searched after searching the current DeclContext instead of searching them as they are found, so that the overall search for candidates is breadth-first instead depth-first. (And now that I've thought of this, I'm going to be updating ScanNamespaceForCorrections to do so since it will further limit the number of namespaces searched).<font color="#000000"><font color="#144FAE"><br>
</font></font></div></div></blockquote><br></div></div><div>As I see it, the main issue for scalability is to avoid having to walk through the declarations in the AST, because that's going to cause massive deserialization from the PCH file. By relying primarily on the walk we have to perform through all of the known identifiers, and then doing targeted lookups into namespaces we know about, we can bound the problem to O(# of candidate identifiers * # of namespaces that are close enough).</div>
<div><br></div><div><span style="white-space:pre-wrap"> </span>- Doug</div></div></blockquote></div><br>
</div></div><span><clang-namespace-aware-typo-corrections2.diff></span></blockquote></div><br></div></div></blockquote></div><br>