This is an archive of the discontinued LLVM Phabricator instance.

[clangd] IncludeCleaner: Support macros
ClosedPublic

Authored by kbobyrev on Oct 25 2021, 6:17 AM.

Details

Summary

Collect the macro definition locations for all the macros used in the main
file.

Diff Detail

Event Timeline

kbobyrev created this revision.Oct 25 2021, 6:17 AM
kbobyrev requested review of this revision.Oct 25 2021, 6:17 AM
sammccall added inline comments.Oct 25 2021, 3:33 PM
clang-tools-extra/clangd/IncludeCleaner.cpp
162

can you add a trace for this and verify that it's not a signficant amount of time on a macro-heavy file (e.g. unittests?)

locateMacroAt wasn't designed to be called in a loop like this, but it seems plausible it'll be fine

162

iterating over all the tokens and examining them individually seems like we're doing too much work.

ParsedAST.MainFileMacros.Names already knows the names of all macros referenced from the main file.
PP.getLocalMacroDirectiveHistory() will give you the last seen definition for a given macro name. If we want, we can also get the previous definitions.

This seems like it should be close enough, and wade through many fewer tokens?

166

locateMacroAt doesn't check that the token is an identifier before trying to fetch its text, maybe do that? either here or there...

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

#define INNER 42
#define OUTER INNER

int answer = OUTER;

(whatever the behavior is, let's add a test for it)

128

#define ANSWER 42
#define SQUARE(X) X * X

int sq = SQUARE(ANSWER);

kbobyrev updated this revision to Diff 382221.Oct 26 2021, 1:36 AM
kbobyrev marked 4 inline comments as done.

Try another strategy to reduce the number of processed tokens (doesn't work in
all cases).

clang-tools-extra/clangd/IncludeCleaner.cpp
162

Hmm, this doesn't really work for "unmaterialized" macros, i.e. ParsedAST::getMacros().Names doesn't have FOO in this case:

// foo.h
#define FOO

// foo.cpp
#define BAR FOO

Is that a bug in CollectMainFileMacros we want to fix or is this intended behavior and we'd need to iterate through the tokens?

kbobyrev updated this revision to Diff 382277.Oct 26 2021, 5:36 AM

Skip the tokens that are not identifiers or weren't defined as macros, attach
tracer.

kbobyrev updated this revision to Diff 382278.Oct 26 2021, 5:49 AM

Rebase on top of main.

kbobyrev updated this revision to Diff 382358.Oct 26 2021, 9:09 AM

Add an elaborate comment on potential future improvements.

sammccall accepted this revision.Oct 27 2021, 12:49 AM

Very nice. As discussed it'd be nice to enhance MainFileMacros at some point but it doesn't seem urgent

clang-tools-extra/clangd/IncludeCleaner.cpp
174

This describes the implementation, not the purpose.

Finds locations of all macros referenced from within the main file.
This includes references that were not yet expanded, like `BAR` in `#define FOO BAR`.
181

I'd rephrase a little:

  • emphasize the data structure MainFileMacros and the data that's missing, rather than the process how it's collected
  • "in the definitions" isn't the essential property, it's rather that they weren't expanded. (e.g. macros uses in PP-disabled sections too). Then macro definitions are an example.
191

this is just a fast-fail for non-identifiers, right? The hadMacroDefinition check is already done in locateMacroAt.

Probably cleaner to move the == tok::identifier check inside locateMacroAt and remove the checks here.

clang-tools-extra/clangd/IncludeCleaner.h
36–41

The start of this sentence is hard to parse: "non-macros" as a noun is vague, and probably needs a comma after. "uses" and "go through" are a bit vague too.

Maybe "A RecursiveASTVisitor finds references to symbols and records their associated locations. These may be macro expansions..."
And "We also examine all identifier tokens in the file in case they reference macros".

Really this is all describing the implementation though, and could be left out of the header entirely...

40

something's happened to the punctuation here

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

Maybe PP-disabled too?

"#define ^FOO\n"
"#define ^BAR

"#if 0
#if FOO
BAR
#endif
#endif
"
This revision is now accepted and ready to land.Oct 27 2021, 12:49 AM
kbobyrev updated this revision to Diff 382549.Oct 27 2021, 1:23 AM
kbobyrev marked 6 inline comments as done.

Address review comments.

kbobyrev updated this revision to Diff 382550.Oct 27 2021, 1:26 AM

Use IncludeCleaner to clean the includes and clean up a little.

This revision was landed with ongoing or failed builds.Oct 27 2021, 1:35 AM
This revision was automatically updated to reflect the committed changes.