ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land.
lg ================ Comment at: clangd/index/SymbolCollector.cpp:127 + // location instead. + if (llvm::StringRef(PrintableLoc(Loc, SM)).startswith("<scratch")) { + FilePathStorage = makeAbsolutePath( ---------------- hokein wrote: > ioeric wrote: > > 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`? > There is no much duplicated code among the cases here (only one line of > generating the SymbolLocation). I prefer to the current way -- it shows the > logic a bit clearer, and also saves the cost of `getSpellingLoc` for the > macro case. I still think there is value in reducing the duplicates. For example, when the interface of `makeAbsolutePath` or `SymbolLocation` changes, we would only need to modify one place rather than two. I wouldn't worry about the cost of `getSpellingLoc` for the macro case since it is a cheap operation for a rare case anyway... ================ Comment at: unittests/clangd/Annotations.h:63 + std::pair<std::size_t, std::size_t> + offsetRange(llvm::StringRef Name = "") const; + ---------------- hokein wrote: > ioeric wrote: > > 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. > I think this is a useful method (probably be used in the future). Putting it > to test matcher, we have to pass `Code` parameter to the matcher > (`LocationOffset(range, Header.code())`) everywhere. Annotation seems to be > the best place. I see. Didn't realize code is needed. ================ Comment at: unittests/clangd/SymbolCollectorTests.cpp:240 + Annotations Main(R"( + #define FF(name) \ + class name##_Test {}; ---------------- Maybe put this in the header file to make the test more meaningful? 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