Record macro references in #ifdef clause.

[VitaNuo]

Summary

Records macro references in #ifdef clauses as ambiguous.

[VitaNuo]

Update

[hokein]

Thanks, this code looks good to me overall. I think we can extend it to handle the Defined, Elifdef, Elifndef cases.

And please add a [include-cleaner] prefix in the commit title.

clang-tools-extra/include-cleaner/lib/Record.cpp

103

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.

[VitaNuo]

Update.

[VitaNuo]

Remove the extra test.

[VitaNuo]

Make explicit RefType default.

[hokein]

clang-tools-extra/include-cleaner/lib/Record.cpp

103

this line exceeds the max 80 character limit, clang-format should fix it.

103

the indentation doesn't look right, running clang-format should fix it.

clang-tools-extra/include-cleaner/unittests/RecordTest.cpp

228

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)

233

the #define Y 2 can be removed, our test doesn't care about it. just use

#ifdef X
#endif

is enough.

248

nit: this can be removed, as the EXPECT_THAT on line 258 covers this.

255

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());.

[VitaNuo]

Add support for #if defined(X), #elifdef, #elifndef

[VitaNuo]

Address review comments. Format and simplify code.

clang-tools-extra/include-cleaner/lib/Record.cpp

103

Right, didn't know default parameter values were possible in C++, thanks.

103

Formatting should be fine now.

clang-tools-extra/include-cleaner/unittests/RecordTest.cpp

228

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.

233

Ok, I'll remove all the fluff.

255

Ok, I've reduced everything to X.

[VitaNuo]

clang-tools-extra/include-cleaner/unittests/RecordTest.cpp

248

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?

[VitaNuo]

Formatting.

[hokein]

clang-tools-extra/include-cleaner/unittests/RecordTest.cpp

235

nit: we can get rid of the main function, it is not needed.

238

nit: this #else line can be removed.

243

The same, can be removed.

248

yeah, the comment was attached to the old snapshot. it is the line 254 now ( ASSERT_THAT(Recorded.MacroReferences, Not(IsEmpty()));).

249

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.

[VitaNuo]

Add -std=c++2b argument to avoid compiler warnings.

clang-tools-extra/include-cleaner/unittests/RecordTest.cpp

248

Ok,removed the check.

249

Thanks!

[VitaNuo]

Simplify test.

[hokein]

Thanks, LGTM. I will commit it for you.

clang-tools-extra/include-cleaner/lib/Record.cpp

void InclusionDirective(SourceLocation Hash, const Token &IncludeTok,
     I.Spelled = SpelledFilename;
     Recorded.Includes.add(I);
   }
 
   void MacroExpands(const Token &MacroName, const MacroDefinition &MD,
                     SourceRange Range, const MacroArgs *Args) override {
     if (!Active)
       return;
-    recordMacroRef(MacroName, *MD.getMacroInfo());
+    recordMacroRef(MacroName, *MD.getMacroInfo(), RefType::Explicit);
   }
 
   void MacroDefined(const Token &MacroName, const MacroDirective *MD) override {
     if (!Active)
       return;
 
     const auto *MI = MD->getMacroInfo();
     // The tokens of a macro definition could refer to a macro.
     // Formally this reference isn't resolved until this macro is expanded,
     // but we want to treat it as a reference anyway.
     for (const auto &Tok : MI->tokens()) {
       auto *II = Tok.getIdentifierInfo();
       // Could this token be a reference to a macro? (Not param to this macro).
       if (!II || !II->hadMacroDefinition() ||
           llvm::is_contained(MI->params(), II))
         continue;
       if (const MacroInfo *MI = PP.getMacroInfo(II))
-        recordMacroRef(Tok, *MI);
+        recordMacroRef(Tok, *MI, RefType::Explicit);
     }
   }
 
   void MacroUndefined(const Token &MacroName, const MacroDefinition &MD,
                       const MacroDirective *) override {
     if (!Active)
       return;
     if (const auto *MI = MD.getMacroInfo())
-      recordMacroRef(MacroName, *MI);
+      recordMacroRef(MacroName, *MI, RefType::Explicit);
+  }
+
+  void Ifdef(SourceLocation Loc, const Token &MacroNameTok,
+                     const MacroDefinition &MD) override {
+    if (!Active)
+      return;
+    if (const auto *MI = MD.getMacroInfo())
+      recordMacroRef(MacroNameTok, *MI, RefType::Ambiguous);
+  }
+
+  void Ifndef(SourceLocation Loc, const Token &MacroNameTok,
+                      const MacroDefinition &MD) override {
+    if (!Active)
+      return;
+    if (const auto *MI = MD.getMacroInfo())
+      recordMacroRef(MacroNameTok, *MI, RefType::Ambiguous);
   }
 
 private:
-  void recordMacroRef(const Token &Tok, const MacroInfo &MI) {
+  void recordMacroRef(const Token &Tok, const MacroInfo &MI, RefType RT) {
     if (MI.isBuiltinMacro())
       return; // __FILE__ is not a reference.
     Recorded.MacroReferences.push_back(
         SymbolReference{Tok.getLocation(),
                         Macro{Tok.getIdentifierInfo(), MI.getDefinitionLoc()},
-                        RefType::Explicit});
+                        RT});
   }
 
   bool Active = false;
   RecordedPP &Recorded;
   const Preprocessor &PP;
   const SourceManager &SM;
 };
 

clang-tools-extra/include-cleaner/unittests/RecordTest.cpp

 //===-- RecordTest.cpp ----------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang-include-cleaner/Record.h"
+#include "clang/Basic/SourceLocation.h"
 #include "clang/Frontend/FrontendAction.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Testing/TestAST.h"
 #include "clang/Tooling/Inclusions/StandardLibrary.h"
-#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Testing/Support/Annotations.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang::include_cleaner {
 namespace {
 using testing::ElementsAre;
if (Ref.Target == OrigX) {
         ADD_FAILURE() << Ref.RefLocation.printToString(SM);
       }
     }
   }
   EXPECT_THAT(RefOffsets, ElementsAreArray(MainFile.points()));
   EXPECT_THAT(ExpOffsets, ElementsAreArray(MainFile.points("exp")));
 }
 
+TEST_F(RecordPPTest, CapturesConditionalMacroRefs) {
+   llvm::Annotations Header(R"cpp(
+    #define Z 2
+  )cpp");
+
+  llvm::Annotations MainFile(R"cpp(
+    #include "header.h"
+
+    #define X 1
+
+    #ifdef ^X
+     #define Y 2
+    #endif
+
+    #ifndef ^Z
+     #define W 3
+    #endif
+  )cpp");
+
+  Inputs.Code = MainFile.code();
+  Inputs.ExtraFiles["header.h"] = Header.code();
+  auto AST = build();
+
+  std::vector<unsigned> RefOffsets;
+  std::vector<std::string> RefNames = {"X", "Z"};
+  SourceManager &SM = AST.sourceManager();
+  ASSERT_THAT(Recorded.MacroReferences, Not(IsEmpty()));
+
+  for (auto I = 0u; I < Recorded.MacroReferences.size(); I++) {
+    const auto &Ref = Recorded.MacroReferences[I];
+    auto [FID, Off] = SM.getDecomposedLoc(Ref.RefLocation);
+    ASSERT_EQ(FID, SM.getMainFileID());
+    EXPECT_EQ(Ref.RT, RefType::Ambiguous);
+    EXPECT_EQ(RefNames[I], Ref.Target.macro().Name->getName());  
+    RefOffsets.push_back(Off);
+  }
+  EXPECT_THAT(RefOffsets, ElementsAreArray(MainFile.points()));
+}
+
+TEST_F(RecordPPTest, CapturesIfNDefMacroRefs) {
+  llvm::Annotations Header(R"cpp(
+    #define Z 2
+  )cpp");
+
+  llvm::Annotations MainFile(R"cpp(
+    #include "header.h"
+
+    #ifndef $def^Z
+     #define W 3
+    #endif
+  )cpp");
+
+  Inputs.Code = MainFile.code();
+  Inputs.ExtraFiles["header.h"] = Header.code();
+  auto AST = build();
+  ASSERT_THAT(Recorded.MacroReferences, Not(IsEmpty()));
+
+  SourceManager &SM = AST.sourceManager();
+  SourceLocation Def = SM.getComposedLoc(SM.getMainFileID(), MainFile.point("def"));
+
+  SymbolReference XRef = Recorded.MacroReferences.front();
+  EXPECT_EQ(XRef.RT, RefType::Ambiguous);
+  EXPECT_EQ("Z", XRef.Target.macro().Name->getName());
+  EXPECT_EQ(XRef.RefLocation, Def);
+}
+
 // Matches an Include* on the specified line;
 MATCHER_P(line, N, "") { return arg->Line == (unsigned)N; }
 
 TEST(RecordedIncludesTest, Match) {
   // We're using synthetic data, but need a FileManager to obtain FileEntry*s.
   // Ensure it doesn't do any actual IO.
   auto FS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
   FileManager FM(FileSystemOptions{});