[PATCH] D66928: [clangd] Collecting main file macro expansion locations in ParsedAST.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!



================
Comment at: clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp:260
+    // Macros from token concatenations included.
+    #define CONCAT(X) X##1()
+    #define MACRO1() 123
----------------
jvikstrom wrote:
> ilya-biryukov wrote:
> > Could we also test?
> > ```
> > #define PREPEND(X) Foo##X
> > ```
> > 
> > It'll probably be the same, but still interesting to see whether it's any differnet.
> So it turns out that the tests weren't actually passing before, must have accidentally forgot to save the file or something before compiling. Anyways tokens from concatenations are not included right now (which for highlighting is probably what we want, we don't highlight types/names that are from macro concatenations either)
> 
> But the reason they are not are because:
> * They do not pass the `isInsideMainFile` check. Their file id is set to something that prints `<scratch space>` when dumped
> * Even if they did pass the check the SourceLocation does not seem to be correct. They return the same SourceLocation as the parent `CONCAT` or `PREPEND`
> 
> Don't know how to fix any of this, and don't know if we actually want to collect these expansions either.
It's definitely ok to not highlight those arguments that get concatenated.
If we wanted to fix this, I think we'd need to detect tokens coming from concatenations separately and finds ways to map them back (not sure if it's possible at all). But let's not bother with this for now.


================
Comment at: clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp:268
+    #include "foo.inc"
+  )cpp");
+  auto TU = TestTU::withCode(TestCase.code());
----------------
I've realized there's one last interesting case that we are missing:
```
#define assert(COND) if (!(COND)) { printf("%s", #COND); exit(0); }

void test(int a) {
  assert(0 <= a); // a should be highlighted
}
```

Could you please add this to the tests? If this works - great; if not - feel free to add a FIXME and land the patch as is, we can fix this later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66928