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

127–128

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

87

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

127–128

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

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

227

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)

232

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

#ifdef X
#endif

is enough.

247

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

254

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

127–128

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

127–128

Formatting should be fine now.

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

227

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.

232

Ok, I'll remove all the fluff.

254

Ok, I've reduced everything to X.

[VitaNuo]

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

247

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

234

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

237

nit: this #else line can be removed.

242

The same, can be removed.

247

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

248

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

247

Ok,removed the check.

248

Thanks!

[VitaNuo]

Simplify test.

[hokein]

Thanks, LGTM. I will commit it for you.

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

public:
   void MacroUndefined(const Token &MacroName, const MacroDefinition &MD,
                       const MacroDirective *) override {
     if (!Active)
       return;
     if (const auto *MI = MD.getMacroInfo())
       recordMacroRef(MacroName, *MI);
   }
 
+  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);
+  }
+
+  void Elifdef(SourceLocation Loc, const Token &MacroNameTok,
+               const MacroDefinition &MD) override {
+    if (!Active)
+      return;
+    if (const auto *MI = MD.getMacroInfo())
+      recordMacroRef(MacroNameTok, *MI, RefType::Ambiguous);
+  }
+
+  void Elifndef(SourceLocation Loc, const Token &MacroNameTok,
+                const MacroDefinition &MD) override {
+    if (!Active)
+      return;
+    if (const auto *MI = MD.getMacroInfo())
+      recordMacroRef(MacroNameTok, *MI, RefType::Ambiguous);
+  }
+
+  void Defined(const Token &MacroNameTok, const MacroDefinition &MD,
+               SourceRange Range) 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 = RefType::Explicit) {
     if (MI.isBuiltinMacro())
       return; // __FILE__ is not a reference.
-    Recorded.MacroReferences.push_back(
-        SymbolReference{Tok.getLocation(),
-                        Macro{Tok.getIdentifierInfo(), MI.getDefinitionLoc()},
-                        RefType::Explicit});
+    Recorded.MacroReferences.push_back(SymbolReference{
+        Tok.getLocation(),
+        Macro{Tok.getIdentifierInfo(), MI.getDefinitionLoc()}, 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 MainFile(R"cpp(
+    #define X 1
+
+    #ifdef ^X
+    #endif
+
+    #if defined(^X)
+    #endif
+
+    #ifndef ^X
+    #endif
+
+    #ifdef Y
+    #elifdef ^X
+    #endif
+
+    #ifndef ^X
+    #elifndef ^X
+    #endif
+  )cpp");
+
+  Inputs.Code = MainFile.code();
+  Inputs.ExtraArgs.push_back("-std=c++2b");
+  auto AST = build();
+
+  std::vector<unsigned> RefOffsets;
+  SourceManager &SM = AST.sourceManager();
+  for (const auto &Ref : Recorded.MacroReferences) {
+    auto [FID, Off] = SM.getDecomposedLoc(Ref.RefLocation);
+    ASSERT_EQ(FID, SM.getMainFileID());
+    EXPECT_EQ(Ref.RT, RefType::Ambiguous);
+    EXPECT_EQ("X", Ref.Target.macro().Name->getName());
+    RefOffsets.push_back(Off);
+  }
+  EXPECT_THAT(RefOffsets, ElementsAreArray(MainFile.points()));
+}
+
 // 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{});