[PATCH] D112447: [clangd] IncludeCleaner: Support 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.

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



================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:174
 
+// Gets the tokens from the main file, iterates through them and adds definition
+// locations for the found macros.
----------------
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`.
```



================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:181
+  // FIXME(kirillbobyrev): We are iterating through all tokens in the main file.
+  // To improve performance, we can improve CollectMainFileMacros: right now
+  // it populates ParsedAST's MainFileMacros with macro references but does not
----------------
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.



================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:191
+       AST.getTokens().spelledTokens(SM.getMainFileID())) {
+    if (Tok.kind() != tok::identifier ||
+        !PP.getIdentifierInfo(Tok.text(SM))->hadMacroDefinition())
----------------
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.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.h:36
 ///
-/// Uses RecursiveASTVisitor to go through main file AST and computes all the
-/// locations used symbols are coming from. Returned locations may be macro
-/// expansions, and are not resolved to their spelling/expansion location. These
-/// locations are later used to determine which headers should be marked as
-/// "used" and "directly used".
+/// - For non-macros uses RecursiveASTVisitor to go through main file AST and
+///   computes all the locations used symbols are coming from. Returned
----------------
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...


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.h:40
+///   or expansion location. These locations are later used to determine which
+///   headers should /   be marked as "used" and "directly used".
+/// - For macros iteratates over \p AST tokens and collects locations of the
----------------
something's happened to the punctuation here


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:127
+          "#define BAR FOO",
+      },
+      {
----------------
Maybe PP-disabled too?

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

"#if 0
#if FOO
BAR
#endif
#endif
"
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112447