[PATCH] D88463: [clangd] Try harder to get accurate ranges for documentSymbols in macros

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Thanks! This seems totally complete now, though I bet there's another case possible somehow!



================
Comment at: clang-tools-extra/clangd/FindSymbols.cpp:157
+  SourceLocation FallbackNameLoc;
+  if (isSpelledInSource(Loc, SM)) {
+    // Prefer the spelling loc, but save the expansion loc as a fallback.
----------------
Hmm... the common file-location case takes a pretty weird path here: both NameLoc and FallbackNameLoc end up set to Loc, and hopefully we never hit the !contains condition (but if we did, we'd just calculate it twice and then hit the second fallback)

Maybe clearer, though a little longer, to handle explicitly like

```
SourceLocation NameLoc = ND.getLocation();
SourceLocation FallbackNameLoc;
if (NameLoc.isMacroLocation()) {
  if (spelled in source) {
    NameLoc = getSpelling
    FallbackNameLoc = getExpanson
  } else {
    NameLoc = getExpansion
  }
}
```

up to you


================
Comment at: clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp:637
+          AllOf(WithName("Test"), SymNameRange(Main.range("expansion2"))),
+          AllOf(WithName("waldo"),
+                SymRangeContains(Main.range("bodyStatement")))));
----------------
nit: despite the "contains" relation being critical, I think it's worth asserting the exact range


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88463/new/

https://reviews.llvm.org/D88463