ioeric added inline comments.
================ Comment at: clangd/index/SymbolCollector.cpp:108 +std::string PrintableLoc(SourceLocation Loc, SourceManager &SM) { + if (Loc.isInvalid()) ---------------- `PrintLoc`? `PrintableLoc` sounds like a checker for whether a location is printable. ================ Comment at: clangd/index/SymbolCollector.cpp:110 + if (Loc.isInvalid()) + return "Invalid location"; + std::string Buffer; ---------------- This works for the use case below (i.e. to get around "<scratch" prefix check) but might cause problem when it's used else where. Please clearly document the behavior, or I'd probably make this a lambda inside `GetSymbolLocation`. ================ Comment at: clangd/index/SymbolCollector.cpp:118 + +SymbolLocation GetSymbolLocation(const NamedDecl* D, SourceManager& SM, + std::string& FilePathStorage) { ---------------- Pull the comments in the patch summary here? ================ Comment at: clangd/index/SymbolCollector.cpp:127 + // location instead. + if (llvm::StringRef(PrintableLoc(Loc, SM)).startswith("<scratch")) { + FilePathStorage = makeAbsolutePath( ---------------- To reduce some code, consider ``` Loc, Start, End = spelling locs; if (D->getLocation().isMacroID() && llvm::StringRef(PrintableLoc(Loc, SM)).startswith("<scratch")) { Loc, Start, End = expension locs; } // Generate SymbolLocation with Loc, Start, End ``` Also, I think you could use `getLocStart` in place of `getLocation`? ================ Comment at: unittests/clangd/Annotations.h:63 + std::pair<std::size_t, std::size_t> + offsetRange(llvm::StringRef Name = "") const; + ---------------- This doesn't seem to be necessary? I think you could use the `range` method above and convert it to offsets in the test matcher. ================ Comment at: unittests/clangd/SymbolCollectorTests.cpp:216-219 + $expansion[[FF(abc)]]; + + #define FF2() \ + $spelling[[class Test {}]]; ---------------- Consider adding another expansion and spelling in the main file (with `IndexMainFiles` enabled), so that we could also check file paths are handled correctly? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42575 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits