[PATCH] D80681: [clang][SourceManager] cache Macro Expansions

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D80681#2096886 <https://reviews.llvm.org/D80681#2096886>, @nickdesaulniers wrote:

> Heh, those were painstakingly written in markdown by hand, using `CC="/usr/bin/time -v clang"` to test.  The weren't statistically significant (N=1), nor were they autogenerated.  My claims about builds of LLVM are similarly non-scientific in terms of statistical significance.


Thanks, I believe it would be still good to provide those numbers for LLVM too (even if they are non-scientific), for the sake of future travellers that want to play with caching logic here.

> I will break this up into 3 patches, and ask you kindly to review once I have them ready.  I will post them here, too, in case other reviewers or subscribers would like to follow along, then I'll Abandon this revision.

thanks a lot, I've provided some details on your libclang interface comment below. but you might want to hold off on dead-code elimination patch, as it is in a quite fragile state and libclang stability is a concern I am not too familiar with.



================
Comment at: clang/include/clang/Basic/SourceManager.h:1747
       return getLoadedSLocEntryByID(ID, Invalid);
-    return getLocalSLocEntry(static_cast<unsigned>(ID), Invalid);
+    return getLocalSLocEntry(static_cast<unsigned>(ID));
   }
----------------
nickdesaulniers wrote:
> kadircet wrote:
> > i think this deserves its separate patch.
> > 
> > We still can satisfy the interface requirements of libclang by introducing another member `getLocalSLocEntryWithInvalid` and change the assertion into an if check, populating `Invalid` in case of failure, what to return is more tricky though it is a const ref, I suppose we can return a dummy entry.
> > 
> > Feel free to keep the newly added calls without an `Invalid` argument, but I wouldn't modify the already existing ones in this patch.
> > the interface requirements of libclang 
> 
> Oh? I must have missed that. Can you tell me more about that interface?  I would have thought this would fail to build if I messed up an interface?
signature requirement is specified in https://github.com/llvm/llvm-project/blob/master/clang/tools/libclang/CIndexInclusionStack.cpp#L21, but that's actually an implementation file. So one could change the details instead and keep libclang API the same (or as mentioned above, you can just introduce a new SourceManager member that will fill in Invalid)

But the more I looked at this code, it started looking more iffy. It's `getLoadedSLocEntry` sibling is operating on a vector, asserting in a couple places and as a fallback just creating a new entry at given index via `lodadedentriesvector[index] = something`without caring about OOB access as it asserted previously. So this whole `Invalid` shenanigan looks ... weird.

Satisfying a const ref return type with an optional `Invalid` flag seems implausible to me :/


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80681/new/

https://reviews.llvm.org/D80681