ilya-biryukov accepted this revision. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. This revision is now accepted and ready to land.
LGTM ================ 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. ---------------- usaxena95 wrote: > ilya-biryukov wrote: > > Won't we get a dangling pointer inside `Refs` after leaving this function? > > MainFileURI gets deallocated at the end, right? > > > Yeah I also thought the same when I saw this at other places too. But the > `Refs.insert(IDToRefs.first, R);` actually does a copy of this string and > owns the copy. Ah, makes sense. Thanks for explaining this. ================ Comment at: clang-tools-extra/clangd/unittests/FileIndexTests.cpp:368 + Index.updateMain(Test.Filename, AST); + // Add test2.cc + TestTU Test2; ---------------- Why do we need two tests? To test the file URI? Setting a custom filename ("test.cc" in our case) should be enough for this. I suggest removing `Test2` completely, it does not seem to test anything new and creates noise. Unless there are things I'm missing that it test. 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