[PATCH] D115243: [clangd] Extend SymbolOrigin, stop serializing it
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Dec 7 17:58:10 PST 2021
sammccall marked 2 inline comments as done.
sammccall added a comment.
In D115243#3176741 <https://reviews.llvm.org/D115243#3176741>, @kadircet wrote:
> I think we also need to update `index/remote/Server.cpp` && `FileSymbols` (and `FileIndex` too).
>
> Regarding updates to `loadIndex`, I actually think it makes sense for that index the always retrieve symbols as `Static` origin, then whoever makes use of that (we always have either a `FileSymbol` or a complete remote-index marshaling on top) should overwrite the origin while either building the final merged index or during marshaling. That would get rid of lots of extra plumbing.
> WDYT?
The problem is these methods yield SymbolSlabs, and the symbols within them are frozen/const. There's no provision to "just tweak some bitfield" - we'd have to copy the whole slab.
In general the const-ness is useful safety: it ensures strings are interned and point to the slab's storage. But it's annoying here.
(The amount of extra plumbing seemed modest enough that I didn't try very hard to hack around this, there may be a neat solution).
================
Comment at: clang-tools-extra/clangd/index/SymbolOrigin.cpp:17
return OS << "unknown";
- constexpr static char Sigils[] = "ADSMIR67";
+ constexpr static char Sigils[] = "AOSMIR67BP012345";
for (unsigned I = 0; I < sizeof(Sigils); ++I)
----------------
kadircet wrote:
> `s/P/9` && `s/6/P`
Whoops, good catch!
================
Comment at: clang-tools-extra/clangd/index/SymbolOrigin.h:26
+ Static = 1 << 2, // From a static, externally-built index.
Merge = 1 << 3, // A non-trivial index merge was performed.
Identifier = 1 << 4, // Raw identifiers in file.
----------------
kadircet wrote:
> do we think this still provides a meaningful signal ?
> It only provides value only when multiple indices of same type was involved in providing the result (and only that single type of indices). I suppose after this patch it can only happen for `SM` (as there are certain remote-index implementations that mark themselves as static) or `RM` (well this is not possible today, but some day we might have a stack of remote-indices).
> As soon as there's a different type of index is involved `M` no longer has any meanings, as it's clear that there was some sort of merge happening (or am I missing something obvious?)
>
> ---
>
> Before this patch situation is a little bit different since `FileIndex` just said `D` for both main file and preamble, but that's changing.
Hmm, we can also get M when merging happens at indexing time due to multiple visits of the same symbol (`DuplicateHandling::Merge`, which is used by background indexing).
I don't have a strong opinion on its usefulness though. IIRC we added it (and origins) when trying to understand the paths various symbols took through indexes. As a debugging tool some redundancy maybe has some sanity-check value, but we could also drop it.
I don't think it has so much to do with this patch (unless you think we should reuse the bit instead of expanding to 16)...
> Before this patch situation is a little bit different since FileIndex just said D for both main file and preamble
...it makes sense that this could have been a reason for the flag, but I don't think it actually was :-)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D115243/new/
https://reviews.llvm.org/D115243
More information about the cfe-commits
mailing list