ilya-biryukov added inline comments.
================ Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:350 + const auto *MainFileEntry = SM.getFileEntryForID(SM.getMainFileID()); + if (!MainFileEntry) + return; ---------------- Does this ever happen? Maybe `assert` it's not null instead? ================ Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:362 + R.Location.End.setColumn(Range.end.character); + R.Location.FileURI = MainFileURI.c_str(); + // FIXME: Add correct RefKind information to MainFileMacros. ---------------- Won't we get a dangling pointer inside `Refs` after leaving this function? MainFileURI gets deallocated at the end, right? ================ Comment at: clang-tools-extra/clangd/index/SymbolCollector.h:108 + void handleMacroOccurence(const MainFileMacros &MacroRefsToIndex); + ---------------- NIT: could you name it in plural? `handleMacros` or `handleMacroOccurences`? It's different from the other version in this regard, which adds only a single occurrence. It's also a good practice to avoid adding overloads to virtual/overriden methods, might get confusing at times. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71406/new/ https://reviews.llvm.org/D71406 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
