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
