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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clangd/index/SymbolCollector.cpp:108
 
+std::string PrintableLoc(SourceLocation Loc, SourceManager &SM) {
+  if (Loc.isInvalid())
----------------
ioeric wrote:
> `PrintLoc`? `PrintableLoc` sounds like a checker for whether a location is printable.
`SourceLocation` has provided `printToString` API already, we don't need to reinvent the wheel now.


================
Comment at: clangd/index/SymbolCollector.cpp:127
+    // location instead.
+    if (llvm::StringRef(PrintableLoc(Loc, SM)).startswith("<scratch")) {
+      FilePathStorage = makeAbsolutePath(
----------------
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.


================
Comment at: unittests/clangd/Annotations.h:63
+  std::pair<std::size_t, std::size_t>
+  offsetRange(llvm::StringRef Name = "") const;
+
----------------
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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42575