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

[Eric Liu via Phabricator via cfe-commits]

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