[cfe-commits] implement all_decls for PCH

[Nick Lewycky]

On 15 April 2012 13:27, Douglas Gregor <dgregor at apple.com> wrote:

>
> 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.
>

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.

> 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.


I disagree. The generator checks:

    // Emit the payload of the table.
    for (unsigned i = 0; i < NumBuckets; ++i) {
      Bucket& B = Buckets[i];
      if (!B.head) continue;

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.

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.
>

Indeed it does.


> Everything else looks great, thanks!
>

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.

Nick
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120415/a21c53f3/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: complete-visibledeclsmap-externalstorage-3.patch
Type: application/octet-stream
Size: 16196 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120415/a21c53f3/attachment.obj>