[PATCH] D138559: Record macro references in #ifdef clause.

[Haojian Wu via Phabricator via cfe-commits]

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