ilya-biryukov added inline comments.
================ Comment at: clang-tools-extra/clangd/index/Background.cpp:482 FileDigest Digest; + bool IsMainFile; }; ---------------- NIT: maybe initialize with `=false` to avoid potential UB. `Digest` seems uninitialized too, could also `={}` for it. Sorry if this was discussed before and I'm repeating myself, I vaguely remember something similar but not sure what the conclusion was. ================ Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:116 + RefSlabs.push_back(FileAndRefs.second.first); + if (FileAndRefs.second.second) + MainFileRefs.push_back(RefSlabs.back().get()); ---------------- Let's use a named struct here? `.second.second` is super-hard to read ================ Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:125 + // Keeps index to symbol in SymsStorage. + llvm::DenseMap<SymbolID, size_t> Merged; for (const auto &Slab : SymbolSlabs) { ---------------- Could we avoid changing the merge logic? I.e. the proposal is to keep the merging code the same and add a loop that counts references at the end. ================ Comment at: clang-tools-extra/clangd/index/FileIndex.h:60 +/// +/// Will count Symbol::References based on number of references in the main +/// files, while building the index with DuplicateHandling::Merge option. ---------------- NIT: this comment seems more appropriate at `buildIndex`, maybe move it there? (It has the `DuplicateHandle` parameter, so it's closer to the context. ================ Comment at: clang-tools-extra/clangd/index/FileIndex.h:66 /// If either is nullptr, corresponding data for \p Path will be removed. + /// If IsMainFile is true, Refs will be used for counting References during + /// merging. ---------------- Maybe call the parameter `CountReferences`? It would narrow down what the code can do with it and make the code easier to understand ================ Comment at: clang-tools-extra/clangd/unittests/FileIndexTests.cpp:49 +MATCHER_P(NumReferences, N, "") { + return arg.References == static_cast<unsigned>(N); +} ---------------- NIT: to avoid warnings, we could use the corresponding suffix at the callsite (e.g. `1u`) instead of casts It's probably ok either way here, but having casts can lead to surprises if one passes negative numbers there Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59481/new/ https://reviews.llvm.org/D59481 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits