This is an archive of the discontinued LLVM Phabricator instance.

[clangd] IncludeCleaner: Handle macros coming from ScratchBuffer
ClosedPublic

Authored by kbobyrev on Oct 13 2021, 2:31 AM.

Diff Detail

Event Timeline

kbobyrev created this revision.Oct 13 2021, 2:31 AM
kbobyrev requested review of this revision.Oct 13 2021, 2:31 AM
kbobyrev updated this revision to Diff 379317.Oct 13 2021, 2:32 AM

Mention the ScratchBuffer explicitly in the test.

kbobyrev updated this revision to Diff 379318.Oct 13 2021, 2:34 AM

Rename the test to have more meaning.

sammccall added inline comments.Oct 13 2021, 5:26 AM
clang-tools-extra/clangd/IncludeCleaner.cpp
123–124

Add a comment

123–124

I'm not 100% sure these should be unconditional. It's *probably* right, but...
Can you add a test of the form:

#define ab x
#define concat(x, y) x##y
int foo(a, b);

and verify that we get the expected set of file IDs when seeding with the location of the VarDecl x?

clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
170

this doesn't seem to test very much, a comment should indicate that this is guarding against a crash.

Ideally we'd (also?) have a test that calls findReferencedFiles and actually assert which IDs we get, rather than run some big blob of code and hope not to crash.

kbobyrev updated this revision to Diff 379366.Oct 13 2021, 7:05 AM
kbobyrev marked 2 inline comments as done.

Address review comments.

kbobyrev added inline comments.Oct 13 2021, 7:12 AM
clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
170

Good point, I just ran into this for D111711:

foo.h

void foo();

foo.cpp

void foo() {}

computeUnusedIncludes() does not return foo.h but it wasn't processed (e.g. its ID was never in the ReferencedFiles in the first place) :(

sammccall accepted this revision.Oct 13 2021, 7:15 AM

LG with the new test

clang-tools-extra/clangd/IncludeCleaner.cpp
123–124

This is about token pasting, nested macro expansions are neither necessary nor sufficient.

clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
198

Nit: I think this should be asserting on FileIDs (i.e. before translation). It's narrower, and the contract is that findReferecedFiles should filter them out already.

198

Comment: no "<scratch space>" FID

199

Comment: should not crash due to "files" missing from include structure

This revision is now accepted and ready to land.Oct 13 2021, 7:15 AM
kbobyrev updated this revision to Diff 379625.Oct 14 2021, 1:37 AM
kbobyrev marked 2 inline comments as done.

Stash

kbobyrev updated this revision to Diff 379661.Oct 14 2021, 4:19 AM
kbobyrev marked 3 inline comments as done.

Resolve review comments.

kbobyrev updated this revision to Diff 379663.Oct 14 2021, 4:23 AM

Fix comment.

This revision was landed with ongoing or failed builds.Oct 14 2021, 4:37 AM
This revision was automatically updated to reflect the committed changes.