[cfe-commits] implement all_decls for PCH

[Douglas Gregor]

On Apr 14, 2012, at 7:34 PM, Nick Lewycky wrote:

> This patch fixes the all_decls iterator to work with PCH, as requested
> by Doug on r153970. As one of the testcases, this includes a complete
> copy of Matthias Kleine's patch here:
>  http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-April/020756.html
> which it also fixes.
> 
> This patch works by exposing the ASTReader's completeVisibleDeclsMap
> API through ExternalASTSource, and also reimplementing it because the
> existing implementation would fail for a CXXRecord that contained
> constructor, destructor or conversion operator.

IIRC, the existing implementation is dead code. It was once used in lieu of chained PCH.

> Doing that necessitated adding a new iterator to the OnDiskHashTable
> called a data_iterator. This is an iterator that never attempts to
> reconstruct an external_key, only using the internal_key. That allows
> us to use it with a Trait that deliberately produces keys without
> enough information to recover the external_key. For example, we don't
> want to store the type of the destructor in the key of the name lookup
> table, which would be necessary to reconstruct the DeclarationName.
> Now, an attempt to instantiate
> ASTDeclContextNameLookupTable::key_iterator or
> ASTDeclContextNameLookupTable::item_iterator will fail at compile
> time.

Very interesting. This is definitely the right way to go, and I have some comments:

+    data_iterator& operator++() {  // Preincrement
+      if (!NumItemsInBucketLeft) {
+        // 'Items' starts with a 16-bit unsigned integer representing the
+        // number of items in this bucket.
+        NumItemsInBucketLeft = io::ReadUnalignedLE16(Ptr);
+      }
+      Ptr += 4; // Skip the hash.

This assumes that each bucket has at least one entry, which isn't guaranteed by the generator. Note that the constructor

+    data_iterator(const unsigned char* const Ptr, unsigned NumEntries,
+                  Info *InfoObj)
+      : Ptr(Ptr), NumItemsInBucketLeft(0), NumEntriesLeft(NumEntries),
+        InfoObj(InfoObj) { }

has the same problem: it needs to scan forward to find the first non-empty bucket.

Index: tools/c-index-test/c-index-test.c
===================================================================
--- tools/c-index-test/c-index-test.c	(revision 154733)
+++ tools/c-index-test/c-index-test.c	(working copy)
@@ -41,6 +41,8 @@
     options &= ~CXTranslationUnit_CacheCompletionResults;
   if (getenv("CINDEXTEST_SKIP_FUNCTION_BODIES"))
     options |= CXTranslationUnit_SkipFunctionBodies;
+  if (getenv("CINDEXTEST_PRECOMPILE_PREAMBLE"))
+    options |= CXTranslationUnit_PrecompiledPreamble;

I don't think we actually need this for testing; setting CINDEXTEST_EDITING should do the trick.

Everything else looks great, thanks!

	- Doug