usaxena95 marked 2 inline comments as done. usaxena95 added inline comments.
================ 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. ---------------- 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. ================ Comment at: clang-tools-extra/clangd/index/SymbolCollector.h:108 + void handleMacroOccurence(const MainFileMacros &MacroRefsToIndex); + ---------------- ilya-biryukov wrote: > 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. > Renamed to `handleMacros` to avoid confusion with the virtual method. 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
