[PATCH] D42575: [clangd] Better handling symbols defined in macros.

[Eric Liu via Phabricator via cfe-commits]

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