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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
