On 15 April 2012 13:27, Douglas Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com">dgregor@apple.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
On Apr 14, 2012, at 7:34 PM, Nick Lewycky wrote:<br>
<br>
> This patch fixes the all_decls iterator to work with PCH, as requested<br>
> by Doug on r153970. As one of the testcases, this includes a complete<br>
> copy of Matthias Kleine's patch here:<br>
> <a href="http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-April/020756.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-April/020756.html</a><br>
> which it also fixes.<br>
><br>
> This patch works by exposing the ASTReader's completeVisibleDeclsMap<br>
> API through ExternalASTSource, and also reimplementing it because the<br>
> existing implementation would fail for a CXXRecord that contained<br>
> constructor, destructor or conversion operator.<br>
<br>
</div>IIRC, the existing implementation is dead code. It was once used in lieu of chained PCH.<br></blockquote><div><br></div><div>There was still a test that used it, through the -chained-include flag, however I think the code was correct. The only time it mattered was when reopening the same decl (not like reopening a C++ namespace, but apparently this is how Objective-C interfaces get extended), and in that case constructors, destructors and conversion functions aren't legal anyways -- so it would've all worked.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> Doing that necessitated adding a new iterator to the OnDiskHashTable<br>
> called a data_iterator. This is an iterator that never attempts to<br>
> reconstruct an external_key, only using the internal_key. That allows<br>
> us to use it with a Trait that deliberately produces keys without<br>
> enough information to recover the external_key. For example, we don't<br>
> want to store the type of the destructor in the key of the name lookup<br>
> table, which would be necessary to reconstruct the DeclarationName.<br>
> Now, an attempt to instantiate<br>
> ASTDeclContextNameLookupTable::key_iterator or<br>
> ASTDeclContextNameLookupTable::item_iterator will fail at compile<br>
> time.<br>
<br>
</div>Very interesting. This is definitely the right way to go, and I have some comments:<br>
<br>
+ data_iterator& operator++() { // Preincrement<br>
+ if (!NumItemsInBucketLeft) {<br>
+ // 'Items' starts with a 16-bit unsigned integer representing the<br>
+ // number of items in this bucket.<br>
+ NumItemsInBucketLeft = io::ReadUnalignedLE16(Ptr);<br>
+ }<br>
+ Ptr += 4; // Skip the hash.<br>
<br>
This assumes that each bucket has at least one entry, which isn't guaranteed by the generator.</blockquote><div><br></div><div>I disagree. The generator checks:</div><div><br></div><div><div> // Emit the payload of the table.</div>
<div> for (unsigned i = 0; i < NumBuckets; ++i) {</div><div> Bucket& B = Buckets[i];</div><div> if (!B.head) continue;</div></div><div><br></div><div>and the only way B.head is non-null is if we've inserted an element into that bucket. I've also added an assertion for B.length != 0 and the tests passed. Which is good, because we don't want to waste disk space writing out empty buckets and if this were possible we should fix it in the generator anyhow.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Note that the constructor<br>
<br>
+ data_iterator(const unsigned char* const Ptr, unsigned NumEntries,<br>
+ Info *InfoObj)<br>
+ : Ptr(Ptr), NumItemsInBucketLeft(0), NumEntriesLeft(NumEntries),<br>
+ InfoObj(InfoObj) { }<br>
<br>
has the same problem: it needs to scan forward to find the first non-empty bucket.<br>
<br>
Index: tools/c-index-test/c-index-test.c<br>
===================================================================<br>
--- tools/c-index-test/c-index-test.c (revision 154733)<br>
+++ tools/c-index-test/c-index-test.c (working copy)<br>
@@ -41,6 +41,8 @@<br>
options &= ~CXTranslationUnit_CacheCompletionResults;<br>
if (getenv("CINDEXTEST_SKIP_FUNCTION_BODIES"))<br>
options |= CXTranslationUnit_SkipFunctionBodies;<br>
+ if (getenv("CINDEXTEST_PRECOMPILE_PREAMBLE"))<br>
+ options |= CXTranslationUnit_PrecompiledPreamble;<br>
<br>
I don't think we actually need this for testing; setting CINDEXTEST_EDITING should do the trick.<br></blockquote><div><br></div><div>Indeed it does.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Everything else looks great, thanks!<br></blockquote><div><br></div><div>Thanks for the quick review! Updated patch attached. This adds the assert to OnDiskHashTableGenerator verifying that no zero-length buckets are emitted and removes OnDiskHashTable:item_iterator as dead code. I also deleted an obsolete comment in DeclContextAllNamesVisitor.</div>
<div><br></div><div>Nick</div><div><br></div></div>