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

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo added inline comments.


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:95
 private:
-  void recordMacroRef(const Token &Tok, const MacroInfo &MI) {
+  void recordMacroRef(const Token &Tok, const MacroInfo &MI, RefType RT) {
     if (MI.isBuiltinMacro())
----------------
hokein wrote:
> nit: we can set a default value (`RefType::Explicit`) for the RT parameter, then we don't need to pass the `RefType::Explicit` in all callsites.
Right, didn't know default parameter values were possible in C++, thanks.


================
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())
----------------
hokein wrote:
> this line exceeds the max 80 character limit, clang-format should fix it.
Formatting should be fine now.


================
Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:228
+  llvm::Annotations MainFile(R"cpp(
+    #include "header.h"
+
----------------
hokein wrote:
> 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)
> 
Ok. I was trying to add some variety, but you're right - it's not the job of this test to check that things are included properly.


================
Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:233
+    #ifdef ^X
+     #define Y 2
+    #endif
----------------
hokein wrote:
> the `#define Y 2` can be removed, our test doesn't care about it. just use
> 
> ```
> #ifdef X
> #endif 
> ```
> is enough.
Ok, I'll remove all the fluff.


================
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);
----------------
hokein wrote:
> 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());`.
> 
> 
Ok, I've reduced everything to X.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138559