[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/unittests/RecordTest.cpp:235
+
+    int main() {
+      #ifdef Y
----------------
nit: we can get rid of the main function, it is not needed.


================
Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:238
+      #elifdef ^X
+      #else 
+      #endif
----------------
nit: this `#else` line can be removed.


================
Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:243
+      #elifndef ^X
+      #else
+      #endif    
----------------
The same, can be removed.


================
Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:249
+
+  Inputs.Code = MainFile.code();
+  auto AST = build();
----------------
The `elifndef` and `elifdef` is a C++2b extension feature, so `Inputs.ExtraArgs.push_back("-std=c++2b");` to get rid of the `[-Wc++2b-extensions]` warning in the testcase.


================
Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:248
+  SourceManager &SM = AST.sourceManager();
+  ASSERT_THAT(Recorded.MacroReferences, Not(IsEmpty()));
+
----------------
VitaNuo wrote:
> hokein wrote:
> > nit: this can be removed, as the EXPECT_THAT on line 258 covers this.
> Sorry, not sure what this comment refers to. Can it be that the line numbering changed due to my newer patch, and this comment does not show up in the correct place anymore?
yeah, the comment was attached to the old snapshot. it is the line 254 now (` ASSERT_THAT(Recorded.MacroReferences, Not(IsEmpty()));`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138559