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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
