hokein added inline comments.
================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:87
+ void Ifdef(SourceLocation Loc, const Token &MacroNameTok,
+ const MacroDefinition &MD) override {
+ if (!Active)
----------------
the indentation doesn't look right, running clang-format should fix it.
================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:103
private:
- void recordMacroRef(const Token &Tok, const MacroInfo &MI) {
+ void recordMacroRef(const Token &Tok, const MacroInfo &MI, RefType RT =
RefType::Explicit) {
if (MI.isBuiltinMacro())
----------------
this line exceeds the max 80 character limit, clang-format should fix it.
================
Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:228
+ llvm::Annotations MainFile(R"cpp(
+ #include "header.h"
+
----------------
I think we can simplify the testcase further -- the header.h is not needed, we
can use the macro `X` in all tests (`#ifdef`, `#ifndef` etc)
================
Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:233
+ #ifdef ^X
+ #define Y 2
+ #endif
----------------
the `#define Y 2` can be removed, our test doesn't care about it. just use
```
#ifdef X
#endif
```
is enough.
================
Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:248
+ SourceManager &SM = AST.sourceManager();
+ ASSERT_THAT(Recorded.MacroReferences, Not(IsEmpty()));
+
----------------
nit: this can be removed, as the EXPECT_THAT on line 258 covers this.
================
Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:255
+ EXPECT_EQ(Ref.RT, RefType::Ambiguous);
+ EXPECT_EQ(RefNames[I], Ref.Target.macro().Name->getName());
+ RefOffsets.push_back(Off);
----------------
we're using the index of the array `Recorded.MacroRefrences` to access another
array, it is fine currently (because both arrays has the same size), but we
will get an out-of-bound-access issue if we add a case `#if defined(X)..`to the
`MainFile`.
If we just use a single macro `X` for all testcases, then the `RefdNames` is
not needed, we check ` EXPECT_EQ("X", Ref.Target.macro().Name->getName());`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138559/new/
https://reviews.llvm.org/D138559
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
