PPCallbacks::MacroUndefined, change signature and add test.

[marsupial]

Summary

The PPCallbacks::MacroUndefined callback is currently insufficient for clients that need to track the MacroDirectives.
This patch adds an additional argument to PPCallbacks::MacroUndefined that is the undef MacroDirective.

[bruno]

Hi,

Can you attach the patch with context? What are you trying to fix by this change? Please also add a testcase.

[marsupial]

I need notification that a macro has been undefined. It's equally as important as a when it was defined.
I'll happily make a test case but want to make sure this would get in before doing so.

[marsupial]

Added test for un-definition callback.

[marsupial]

ping

[manmanren]

Please update the patch with context: http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Thanks,
Manman

unittests/Basic/SourceManagerTest.cpp

251 ↗

(On Diff #88836)

Can we make this an enum?

[marsupial]

ping

[bruno]

LGTM!

cfe/trunk/include/clang/Lex/PPCallbacks.h

virtual void MacroExpands(const Token &MacroNameTok,
                             const MacroArgs *Args) {}
 
   /// \brief Hook called whenever a macro definition is seen.
   virtual void MacroDefined(const Token &MacroNameTok,
                             const MacroDirective *MD) {
   }
 
   /// \brief Hook called whenever a macro \#undef is seen.
+  /// \param Token The active Token
+  /// \param MD A MacroDefinition for the named macro.
+  /// \param Undef New MacroDirective if the macro was defined, null otherwise.
   ///
   /// MD is released immediately following this callback.
   virtual void MacroUndefined(const Token &MacroNameTok,
-                              const MacroDefinition &MD) {
+                              const MacroDefinition &MD,
+                              const MacroDirective *Undef) {
   }
   
   /// \brief Hook called whenever the 'defined' operator is seen.
   /// \param MD The MacroDirective if the name was a macro, null otherwise.
   virtual void Defined(const Token &MacroNameTok, const MacroDefinition &MD,
                        SourceRange Range) {
   }
   
public:
   }
 
   void MacroExpands(const Token &MacroNameTok, const MacroDefinition &MD,
                     SourceRange Range, const MacroArgs *Args) override {
     First->MacroExpands(MacroNameTok, MD, Range, Args);
     Second->MacroExpands(MacroNameTok, MD, Range, Args);
   }
 
-  void MacroDefined(const Token &MacroNameTok, const MacroDirective *MD) override {
+  void MacroDefined(const Token &MacroNameTok,
+                    const MacroDirective *MD) override {
     First->MacroDefined(MacroNameTok, MD);
     Second->MacroDefined(MacroNameTok, MD);
   }
 
   void MacroUndefined(const Token &MacroNameTok,
-                      const MacroDefinition &MD) override {
-    First->MacroUndefined(MacroNameTok, MD);
-    Second->MacroUndefined(MacroNameTok, MD);
+                      const MacroDefinition &MD,
+                      const MacroDirective *Undef) override {
+    First->MacroUndefined(MacroNameTok, MD, Undef);
+    Second->MacroUndefined(MacroNameTok, MD, Undef);
   }
 
   void Defined(const Token &MacroNameTok, const MacroDefinition &MD,
                SourceRange Range) override {
     First->Defined(MacroNameTok, MD, Range);
     Second->Defined(MacroNameTok, MD, Range);
   }
 

cfe/trunk/include/clang/Lex/PreprocessingRecord.h

public:
     const std::vector<SourceRange> &getSkippedRanges() const {
       return SkippedRanges;
     }
         
   private:
     void MacroExpands(const Token &Id, const MacroDefinition &MD,
                       SourceRange Range, const MacroArgs *Args) override;
     void MacroDefined(const Token &Id, const MacroDirective *MD) override;
-    void MacroUndefined(const Token &Id, const MacroDefinition &MD) override;
+    void MacroUndefined(const Token &Id, const MacroDefinition &MD,
+                        const MacroDirective *Undef) override;
     void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
                             StringRef FileName, bool IsAngled,
                             CharSourceRange FilenameRange,
                             const FileEntry *File, StringRef SearchPath,
                             StringRef RelativePath,
                             const Module *Imported) override;
     void Ifdef(SourceLocation Loc, const Token &MacroNameTok,
                const MacroDefinition &MD) override;

cfe/trunk/lib/Frontend/PrintPreprocessedOutput.cpp

public:
   void HandleNewlinesInToken(const char *TokStr, unsigned Len);
 
   /// MacroDefined - This hook is called whenever a macro definition is seen.
   void MacroDefined(const Token &MacroNameTok,
                     const MacroDirective *MD) override;
 
   /// MacroUndefined - This hook is called whenever a macro #undef is seen.
   void MacroUndefined(const Token &MacroNameTok,
-                      const MacroDefinition &MD) override;
+                      const MacroDefinition &MD,
+                      const MacroDirective *Undef) override;
 };
 }  // end anonymous namespace
 
 void PrintPPOutputPPCallbacks::WriteLineInfo(unsigned LineNo,
                                              const char *Extra,
                                              unsigned ExtraLen) {
   startNewLineIfNeeded(/*ShouldUpdateCurrentLine=*/false);
 
if (!DumpDefines ||
       MI->isBuiltinMacro()) return;
 
   MoveToLine(MI->getDefinitionLoc());
   PrintMacroDefinition(*MacroNameTok.getIdentifierInfo(), *MI, PP, OS);
   setEmittedDirectiveOnThisLine();
 }
 
 void PrintPPOutputPPCallbacks::MacroUndefined(const Token &MacroNameTok,
-                                              const MacroDefinition &MD) {
+                                              const MacroDefinition &MD,
+                                              const MacroDirective *Undef) {
   // Only print out macro definitions in -dD mode.
   if (!DumpDefines) return;
 
   MoveToLine(MacroNameTok.getLocation());
   OS << "#undef " << MacroNameTok.getIdentifierInfo()->getName();
   setEmittedDirectiveOnThisLine();
 }
 

cfe/trunk/lib/Lex/PPDirectives.cpp

if (MacroNameTok.is(tok::eod))
     return;
 
   // Check to see if this is the last token on the #undef line.
   CheckEndOfDirective("undef");
 
   // Okay, we have a valid identifier to undef.
   auto *II = MacroNameTok.getIdentifierInfo();
   auto MD = getMacroDefinition(II);
+  UndefMacroDirective *Undef = nullptr;
   
-  // If the callbacks want to know, tell them about the macro #undef.
-  // Note: no matter if the macro was defined or not.
-  if (Callbacks)
-    Callbacks->MacroUndefined(MacroNameTok, MD);
-
-  // If the macro is not defined, this is a noop undef, just return.
-  const MacroInfo *MI = MD.getMacroInfo();
-  if (!MI)
-    return;
-
+  // If the macro is not defined, this is a noop undef.
+  if (const MacroInfo *MI = MD.getMacroInfo()) {
     if (!MI->isUsed() && MI->isWarnIfUnused())
       Diag(MI->getDefinitionLoc(), diag::pp_macro_not_used);
 
     if (MI->isWarnIfUnused())
       WarnUnusedMacroLocs.erase(MI->getDefinitionLoc());
 
-  appendMacroDirective(MacroNameTok.getIdentifierInfo(),
-                       AllocateUndefMacroDirective(MacroNameTok.getLocation()));
+    Undef = AllocateUndefMacroDirective(MacroNameTok.getLocation());
+  }
+
+  // If the callbacks want to know, tell them about the macro #undef.
+  // Note: no matter if the macro was defined or not.
+  if (Callbacks)
+    Callbacks->MacroUndefined(MacroNameTok, MD, Undef);
+
+  if (Undef)
+    appendMacroDirective(II, Undef);
 }
 
 //===----------------------------------------------------------------------===//
 // Preprocessor Conditional Directive Handling.
 //===----------------------------------------------------------------------===//
 
 /// HandleIfdefDirective - Implements the \#ifdef/\#ifndef directive.  isIfndef
 /// is true when this is a \#ifndef directive.  ReadAnyTokensBeforeDirective is

cfe/trunk/lib/Lex/PreprocessingRecord.cpp

void PreprocessingRecord::MacroDefined(const Token &Id,
   SourceRange R(MI->getDefinitionLoc(), MI->getDefinitionEndLoc());
   MacroDefinitionRecord *Def =
       new (*this) MacroDefinitionRecord(Id.getIdentifierInfo(), R);
   addPreprocessedEntity(Def);
   MacroDefinitions[MI] = Def;
 }
 
 void PreprocessingRecord::MacroUndefined(const Token &Id,
-                                         const MacroDefinition &MD) {
+                                         const MacroDefinition &MD,
+                                         const MacroDirective *Undef) {
   MD.forAllDefinitions([&](MacroInfo *MI) { MacroDefinitions.erase(MI); });
 }
 
 void PreprocessingRecord::InclusionDirective(
     SourceLocation HashLoc,
     const clang::Token &IncludeTok,
     StringRef FileName,
     bool IsAngled,

cfe/trunk/tools/libclang/Indexing.cpp

public:
   }
 
   /// MacroDefined - This hook is called whenever a macro definition is seen.
   void MacroDefined(const Token &Id, const MacroDirective *MD) override {}
 
   /// MacroUndefined - This hook is called whenever a macro #undef is seen.
   /// MI is released immediately following this callback.
   void MacroUndefined(const Token &MacroNameTok,
-                      const MacroDefinition &MD) override {}
+                      const MacroDefinition &MD,
+                      const MacroDirective *UD) override {}
 
   /// MacroExpands - This is called by when a macro invocation is found.
   void MacroExpands(const Token &MacroNameTok, const MacroDefinition &MD,
                     SourceRange Range, const MacroArgs *Args) override {}
 
   /// SourceRangeSkipped - This hook is called when a source range is skipped.
   /// \param Range The SourceRange that was skipped. The range begins at the
   /// #if/#else directive and ends after the #endif/#else directive.

cfe/trunk/unittests/Basic/SourceManagerTest.cpp

TEST_F(SourceManagerTest, getMacroArgExpandedLocation) {
   EXPECT_EQ(loc2, toks[1].getLocation());
   EXPECT_EQ(loc3, toks[2].getLocation());
   EXPECT_TRUE(defLoc2.isFileID());
 }
 
 namespace {
 
 struct MacroAction {
+  enum Kind { kExpansion, kDefinition, kUnDefinition};
+
   SourceLocation Loc;
   std::string Name;
-  bool isDefinition; // if false, it is expansion.
-  
-  MacroAction(SourceLocation Loc, StringRef Name, bool isDefinition)
-    : Loc(Loc), Name(Name), isDefinition(isDefinition) { }
+  unsigned MAKind : 3;
+
+  MacroAction(SourceLocation Loc, StringRef Name, unsigned K)
+    : Loc(Loc), Name(Name), MAKind(K) { }
+  bool isExpansion() const { return MAKind == kExpansion; }
+  bool isDefinition() const { return MAKind & kDefinition; }
+  bool isUnDefinition() const { return MAKind & kUnDefinition; }
 };
 
 class MacroTracker : public PPCallbacks {
   std::vector<MacroAction> &Macros;
 
 public:
   explicit MacroTracker(std::vector<MacroAction> &Macros) : Macros(Macros) { }
 
   void MacroDefined(const Token &MacroNameTok,
                     const MacroDirective *MD) override {
     Macros.push_back(MacroAction(MD->getLocation(),
                                  MacroNameTok.getIdentifierInfo()->getName(),
-                                 true));
+                                 MacroAction::kDefinition));
+  }
+  void MacroUndefined(const Token &MacroNameTok,
+                      const MacroDefinition &MD,
+                      const MacroDirective  *UD) override {
+    Macros.push_back(
+        MacroAction(UD ? UD->getLocation() : SourceLocation(),
+                    MacroNameTok.getIdentifierInfo()->getName(),
+                    UD ? MacroAction::kDefinition | MacroAction::kUnDefinition
+                       : MacroAction::kUnDefinition));
   }
   void MacroExpands(const Token &MacroNameTok, const MacroDefinition &MD,
                     SourceRange Range, const MacroArgs *Args) override {
     Macros.push_back(MacroAction(MacroNameTok.getLocation(),
                                  MacroNameTok.getIdentifierInfo()->getName(),
-                                 false));
+                                 MacroAction::kExpansion));
   }
 };
 
 }
 
 TEST_F(SourceManagerTest, isBeforeInTranslationUnitWithMacroInInclude) {
   const char *header =
-    "#define MACRO_IN_INCLUDE 0\n";
+    "#define MACRO_IN_INCLUDE 0\n"
+    "#define MACRO_DEFINED\n"
+    "#undef MACRO_DEFINED\n"
+    "#undef MACRO_UNDEFINED\n";
 
   const char *main =
     "#define M(x) x\n"
     "#define INC \"/test-header.h\"\n"
     "#include M(INC)\n"
     "#define INC2 </test-header.h>\n"
     "#include M(INC2)\n";
 
while (1) {
     if (tok.is(tok::eof))
       break;
     toks.push_back(tok);
   }
 
   // Make sure we got the tokens that we expected.
   ASSERT_EQ(0U, toks.size());
 
-  ASSERT_EQ(9U, Macros.size());
+  ASSERT_EQ(15U, Macros.size());
   // #define M(x) x
-  ASSERT_TRUE(Macros[0].isDefinition);
+  ASSERT_TRUE(Macros[0].isDefinition());
   ASSERT_EQ("M", Macros[0].Name);
   // #define INC "/test-header.h"
-  ASSERT_TRUE(Macros[1].isDefinition);
+  ASSERT_TRUE(Macros[1].isDefinition());
   ASSERT_EQ("INC", Macros[1].Name);
   // M expansion in #include M(INC)
-  ASSERT_FALSE(Macros[2].isDefinition);
+  ASSERT_FALSE(Macros[2].isDefinition());
   ASSERT_EQ("M", Macros[2].Name);
   // INC expansion in #include M(INC)
-  ASSERT_FALSE(Macros[3].isDefinition);
+  ASSERT_TRUE(Macros[3].isExpansion());
   ASSERT_EQ("INC", Macros[3].Name);
   // #define MACRO_IN_INCLUDE 0
-  ASSERT_TRUE(Macros[4].isDefinition);
+  ASSERT_TRUE(Macros[4].isDefinition());
   ASSERT_EQ("MACRO_IN_INCLUDE", Macros[4].Name);
+  // #define MACRO_DEFINED
+  ASSERT_TRUE(Macros[5].isDefinition());
+  ASSERT_FALSE(Macros[5].isUnDefinition());
+  ASSERT_EQ("MACRO_DEFINED", Macros[5].Name);
+  // #undef MACRO_DEFINED
+  ASSERT_TRUE(Macros[6].isDefinition());
+  ASSERT_TRUE(Macros[6].isUnDefinition());
+  ASSERT_EQ("MACRO_DEFINED", Macros[6].Name);
+  // #undef MACRO_UNDEFINED
+  ASSERT_FALSE(Macros[7].isDefinition());
+  ASSERT_TRUE(Macros[7].isUnDefinition());
+  ASSERT_EQ("MACRO_UNDEFINED", Macros[7].Name);
   // #define INC2 </test-header.h>
-  ASSERT_TRUE(Macros[5].isDefinition);
-  ASSERT_EQ("INC2", Macros[5].Name);
+  ASSERT_TRUE(Macros[8].isDefinition());
+  ASSERT_EQ("INC2", Macros[8].Name);
   // M expansion in #include M(INC2)
-  ASSERT_FALSE(Macros[6].isDefinition);
-  ASSERT_EQ("M", Macros[6].Name);
+  ASSERT_FALSE(Macros[9].isDefinition());
+  ASSERT_EQ("M", Macros[9].Name);
   // INC2 expansion in #include M(INC2)
-  ASSERT_FALSE(Macros[7].isDefinition);
-  ASSERT_EQ("INC2", Macros[7].Name);
+  ASSERT_TRUE(Macros[10].isExpansion());
+  ASSERT_EQ("INC2", Macros[10].Name);
   // #define MACRO_IN_INCLUDE 0
-  ASSERT_TRUE(Macros[8].isDefinition);
-  ASSERT_EQ("MACRO_IN_INCLUDE", Macros[8].Name);
+  ASSERT_TRUE(Macros[11].isDefinition());
+  ASSERT_EQ("MACRO_IN_INCLUDE", Macros[11].Name);
 
   // The INC expansion in #include M(INC) comes before the first
   // MACRO_IN_INCLUDE definition of the included file.
   EXPECT_TRUE(SourceMgr.isBeforeInTranslationUnit(Macros[3].Loc, Macros[4].Loc));
 
   // The INC2 expansion in #include M(INC2) comes before the second
   // MACRO_IN_INCLUDE definition of the included file.
-  EXPECT_TRUE(SourceMgr.isBeforeInTranslationUnit(Macros[7].Loc, Macros[8].Loc));
+  EXPECT_TRUE(SourceMgr.isBeforeInTranslationUnit(Macros[10].Loc, Macros[11].Loc));
 }
 
 #endif
 
 } // anonymous namespace