Page MenuHomePhabricator

[clangd] Collecting main file macro expansion locations in ParsedAST.
ClosedPublic

Authored by jvikstrom on Aug 29 2019, 12:59 AM.

Details

Summary

TokenBuffer does not collect macro expansions inside macro arguments which is needed for semantic higlighting. Therefore collects macro expansions in the main file in a PPCallback when building the ParsedAST instead.

Diff Detail

Repository
rL LLVM

Event Timeline

jvikstrom created this revision.Aug 29 2019, 12:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 29 2019, 12:59 AM

Looks great, just requests for comments and more test-cases from my side

clang-tools-extra/clangd/ClangdUnit.h
148 ↗(On Diff #217789)

NIT: clarify that this is the start location of the identifier that was macro-expanded.
"expansion location" is an overloaded term

149 ↗(On Diff #217789)

Could you please document whether these include:

  1. locations outside the main file
  2. locations from inside other macro expansions, i.e. those that have isMacroID() == true.
clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
248 ↗(On Diff #217789)

Could you add a few more interesting cases?

  1. Macros outside the main file and the preamble:
// foo.inc
int a = ID(1);

// foo.cpp
#define ID(X) X

int b;
#include "foo.inc"
  1. macro expansions from token concatenations
#define FOO(X) X##1()
#define MACRO1() 123

int a = FOO(MACRO);
  1. Macro names inside other macros:
#define FOO BAR
#define BAR 1


int a = FOO; // should BAR at line 1 be highlighted?
  1. #include not part of the preamble:
#define FOO 1

// Preamble ends here.
int a = 10;
#include "some_file_with_macros.h" // <-- should not get any macros from here
264 ↗(On Diff #217789)

NIT: maybe use push_back and for-each loop, the resulting code should be simpler and readability is more important for the test code than performance:

std::vector<Position> Positions;
for (SourceLocation Loc : AST.getMainFileMacros())
  Positions.push_back(sourceLocToPosition(Loc));
jvikstrom updated this revision to Diff 217811.Aug 29 2019, 3:06 AM
jvikstrom marked 5 inline comments as done.

Clarified comments. Added tests. Not getting expansions inside other macro expansions.

jvikstrom added inline comments.Aug 29 2019, 3:11 AM
clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
248 ↗(On Diff #217789)

Does clangd handle .inc files differently from .h? Because if it doesn't shouldn't ` case 1 cover case 4 as well ?

ilya-biryukov marked an inline comment as done.Aug 29 2019, 5:30 AM
ilya-biryukov added inline comments.
clang-tools-extra/clangd/ClangdUnit.cpp
107 ↗(On Diff #217811)

Maybe make this part of CollectMainFileMacros? It looks like a natural fit there and we won't need another instance of PPCallbacks.

120 ↗(On Diff #217811)

I believe getSpellingLoc is a no-op if isMacroID() is false. Maybe simplify to:

if (!L.isMacroID() && isInsideMainFile(L, SM))
  ...
clang-tools-extra/clangd/ClangdUnit.h
119 ↗(On Diff #217811)

NIT: return llvm::ArrayRef<SourceLocation> instead of const vector

119 ↗(On Diff #217811)

NIT: maybe shorten the name to getMainFileExpansions?

150 ↗(On Diff #217811)

NIT: a comment like this or a similar one is probably also useful in the public accessor.

clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
248 ↗(On Diff #217789)

Right, sorry, was rushing to finish a comment. .inc and .h are handled exactly the same.
There is only a difference if they are in the preamble or not.

260 ↗(On Diff #217811)

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.

jvikstrom marked an inline comment as done.Aug 29 2019, 7:26 AM
jvikstrom added inline comments.
clang-tools-extra/clangd/ClangdUnit.cpp
107 ↗(On Diff #217811)

But CollectMainFileMacros is only used for buildPreamble.
I think this one needs to run in ParsedAST::build?

Is it safe to add a CollectMainFileMacros in ParsedAST::build`?

ilya-biryukov added inline comments.Aug 29 2019, 7:57 AM
clang-tools-extra/clangd/ClangdUnit.cpp
107 ↗(On Diff #217811)

Ah, good point, LG then.
Could you put a small comment beside the class that explicitly mentions this?
It's obvious when reading through the whole code, but not obvious when peeking at different parts of it from a distance.

Something like

// CollectMainFileMacros and CollectMainFileMacroExpansions are two different classes because
// the latter is only used when building preamble and the former only when building the AST for the main file.
jvikstrom updated this revision to Diff 217920.Aug 29 2019, 9:56 AM
jvikstrom marked 4 inline comments as done.

Added test for prepending concatenations. Also added made tests pass.

jvikstrom marked an inline comment as done.Aug 29 2019, 10:01 AM
jvikstrom added inline comments.
clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
260 ↗(On Diff #217811)

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.

ilya-biryukov accepted this revision.Aug 30 2019, 2:05 AM
ilya-biryukov marked an inline comment as done.

LGTM, thanks!

clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
260 ↗(On Diff #217811)

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.

268 ↗(On Diff #217920)

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.

This revision is now accepted and ready to land.Aug 30 2019, 2:05 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 30 2019, 2:32 AM