sammccall added a comment.
Mostly LG.
I think we can simplify in a couple of places, and you should decide if this
patch is *implementing* the new index operation or not. (Currently it
implements it for 1/3 implementations, which doesn't seem that useful).
================
Comment at: clangd/index/FileIndex.cpp:111
+ llvm::function_ref<void(const SymbolOccurrence &)> Callback) const {
+ // FIXME(hokein): implement it.
+}
----------------
add a log?
================
Comment at: clangd/index/FileIndex.cpp:111
+ llvm::function_ref<void(const SymbolOccurrence &)> Callback) const {
+ // FIXME(hokein): implement it.
+}
----------------
sammccall wrote:
> add a log?
Can we either implement this for FileIndex/MemIndex/MergedIndex, or none of
them?
ATM the scope of the patch is unclear. If we don't implement it, then no need
to add Slab etc. If we do, then we get 3 callsites to make sure all that stuff
actually works well :-)
================
Comment at: clangd/index/Index.cpp:138
+raw_ostream &operator<<(raw_ostream &OS, SymbolOccurrenceKind K) {
+ if (K == SymbolOccurrenceKind::Unknown)
+ return OS << "unknown";
----------------
this is a bitfield, there are at least 8 legal values.
If you intend each occurrence to have only one type, and only the filter to
have multiple values, these should probably be different types.
Do we actually need operator<< in this patch, or is the default (print the
number) OK for tests?
================
Comment at: clangd/index/Index.h:313
+ // The location of the occurrence.
+ // WARNING: Location does not own the underlying data - FileURI are owned by
a
+ // SymbolOccurrenceSlab. Copies are shallow.
----------------
Hmm, remove the bit about slab? That's only going to be true in a smaller set
of cases than with Symbol.
Not owning the data is a good note, but probably belongs on the struct rather
than the member.
================
Comment at: clangd/index/Index.h:345
+
+ // A mutable container that can 'freeze' to SymbolOccurrenceSlab.
+ // The frozen OccurrenceSlab will use less memory.
----------------
Again, I'm not sure the builder/slab split is justifiable here. Can we start
without it? Cost is ~8 bytes per unique filename, which should be pretty
trivial.
================
Comment at: clangd/index/Index.h:429
+ ///
+ /// The API is not responsible to rank results.
+ /// The returned result must be deep-copied if it's used outside Callback.
----------------
nit: `Results are returned in arbitrary order.`?
(Usually we want to document from the POV of a caller, rather than an
implementation)
================
Comment at: clangd/index/MemIndex.cpp:97
+ for (const auto& Occurrence : Occurrences.find(ID)) {
+ if (static_cast<uint32_t>(Req.Filter & Occurrence.Kind)) {
+ Callback(Occurrence);
----------------
cast to bool rather than int32_t?
================
Comment at: clangd/index/MemIndex.h:26
+ void build(std::shared_ptr<std::vector<const Symbol *>> Symbols,
+ SymbolOccurrenceSlab OccurrenceSlab = {});
----------------
nit: I'm not sure default args make sense here unless we intend for this to be
forever an optional feature
================
Comment at: clangd/index/MemIndex.h:29
/// \brief Build index from a symbol slab.
static std::unique_ptr<SymbolIndex> build(SymbolSlab Slab);
----------------
occurrences here too
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D49658
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
