Merge topic 'dev/fix-variable-watch-crash'

6aa0c21 variable_watch: Add test for watching a variable multiple times
b86e37c variable_watch: Check newValue for NULL
f9bb20f variable_watch: Don't share memory for callbacks
05dad99 variable_watch: Fix a typo in the error message
00ce12a variable_watch: Prevent making extra entries in the watch map
34b397e variable_watch: Allow specifying the data to match in RemoveWatch
e43e207 variable_watch: Match client_data when finding duplicates
0d6acb1 variable_watch: Add a deleter for the client data
fc7c3b4 variable_watch: Store client data as pointers

Source/cmVariableWatch.cxx

@@ -37,37 +37,63 @@ cmVariableWatch::cmVariableWatch()
 
 cmVariableWatch::~cmVariableWatch()
 {
+  cmVariableWatch::StringToVectorOfPairs::iterator svp_it;
+
+  for ( svp_it = this->WatchMap.begin();
+        svp_it != this->WatchMap.end(); ++svp_it )
+    {
+    cmVariableWatch::VectorOfPairs::iterator p_it;
+
+    for ( p_it = svp_it->second.begin();
+          p_it != svp_it->second.end(); ++p_it )
+      {
+      delete *p_it;
+      }
+    }
 }
 
-void cmVariableWatch::AddWatch(const std::string& variable,
+bool cmVariableWatch::AddWatch(const std::string& variable,
-                               WatchMethod method, void* client_data /*=0*/)
+                               WatchMethod method, void* client_data /*=0*/,
+                               DeleteData delete_data /*=0*/)
 {
-  cmVariableWatch::Pair p;
+  cmVariableWatch::Pair* p = new cmVariableWatch::Pair;
-  p.Method = method;
+  p->Method = method;
-  p.ClientData = client_data;
+  p->ClientData = client_data;
+  p->DeleteDataCall = delete_data;
   cmVariableWatch::VectorOfPairs* vp = &this->WatchMap[variable];
   cmVariableWatch::VectorOfPairs::size_type cc;
   for ( cc = 0; cc < vp->size(); cc ++ )
     {
-    cmVariableWatch::Pair* pair = &(*vp)[cc];
+    cmVariableWatch::Pair* pair = (*vp)[cc];
-    if ( pair->Method == method )
+    if ( pair->Method == method &&
+         client_data && client_data == pair->ClientData)
       {
-      (*vp)[cc] = p;
+      // Callback already exists
-      return;
+      return false;
       }
     }
   vp->push_back(p);
+  return true;
 }
 
 void cmVariableWatch::RemoveWatch(const std::string& variable,
-                                  WatchMethod method)
+                                  WatchMethod method,
+                                  void* client_data /*=0*/)
 {
+  if ( !this->WatchMap.count(variable) )
+    {
+    return;
+    }
   cmVariableWatch::VectorOfPairs* vp = &this->WatchMap[variable];
   cmVariableWatch::VectorOfPairs::iterator it;
   for ( it = vp->begin(); it != vp->end(); ++it )
     {
-    if ( it->Method == method )
+    if ( (*it)->Method == method &&
+         // If client_data is NULL, we want to disconnect all watches against
+         // the given method; otherwise match ClientData as well.
+         (!client_data || (client_data == (*it)->ClientData)))
       {
+      delete *it;
       vp->erase(it);
       return;
       }
@@ -87,7 +113,7 @@ void  cmVariableWatch::VariableAccessed(const std::string& variable,
     cmVariableWatch::VectorOfPairs::const_iterator it;
     for ( it = vp->begin(); it != vp->end(); it ++ )
       {
-      it->Method(variable, access_type, it->ClientData,
+      (*it)->Method(variable, access_type, (*it)->ClientData,
         newValue, mf);
       }
     }

Source/cmVariableWatch.h

@@ -26,6 +26,7 @@ class cmVariableWatch
 public:
   typedef void (*WatchMethod)(const std::string& variable, int access_type,
     void* client_data, const char* newValue, const cmMakefile* mf);
+  typedef void (*DeleteData)(void* client_data);
 
   cmVariableWatch();
   ~cmVariableWatch();
@@ -33,9 +34,10 @@ public:
   /**
    * Add watch to the variable
    */
-  void AddWatch(const std::string& variable, WatchMethod method,
+  bool AddWatch(const std::string& variable, WatchMethod method,
-                void* client_data=0);
+                void* client_data=0, DeleteData delete_data=0);
-  void RemoveWatch(const std::string& variable, WatchMethod method);
+  void RemoveWatch(const std::string& variable, WatchMethod method,
+                   void* client_data=0);
 
   /**
    * This method is called when variable is accessed
@@ -67,10 +69,18 @@ protected:
   {
     WatchMethod Method;
     void*        ClientData;
-    Pair() : Method(0), ClientData(0) {}
+    DeleteData   DeleteDataCall;
+    Pair() : Method(0), ClientData(0), DeleteDataCall(0) {}
+    ~Pair()
+      {
+      if (this->DeleteDataCall && this->ClientData)
+        {
+        this->DeleteDataCall(this->ClientData);
+        }
+      }
   };
 
-  typedef std::vector< Pair > VectorOfPairs;
+  typedef std::vector< Pair* > VectorOfPairs;
   typedef std::map<cmStdString, VectorOfPairs > StringToVectorOfPairs;
 
   StringToVectorOfPairs WatchMap;

Source/cmVariableWatchCommand.cxx

@@ -13,20 +13,104 @@
 
 #include "cmVariableWatch.h"
 
+//----------------------------------------------------------------------------
+struct cmVariableWatchCallbackData
+{
+  bool InCallback;
+  std::string Command;
+};
+
 //----------------------------------------------------------------------------
 static void cmVariableWatchCommandVariableAccessed(
   const std::string& variable, int access_type, void* client_data,
   const char* newValue, const cmMakefile* mf)
 {
-  cmVariableWatchCommand* command
+  cmVariableWatchCallbackData* data
-    = static_cast<cmVariableWatchCommand*>(client_data);
+    = static_cast<cmVariableWatchCallbackData*>(client_data);
-  command->VariableAccessed(variable, access_type, newValue, mf);
+
+  if ( data->InCallback )
+    {
+    return;
+    }
+  data->InCallback = true;
+
+  cmListFileFunction newLFF;
+  cmListFileArgument arg;
+  bool processed = false;
+  const char* accessString = cmVariableWatch::GetAccessAsString(access_type);
+  const char* currentListFile = mf->GetDefinition("CMAKE_CURRENT_LIST_FILE");
+
+  /// Ultra bad!!
+  cmMakefile* makefile = const_cast<cmMakefile*>(mf);
+
+  std::string stack = makefile->GetProperty("LISTFILE_STACK");
+  if ( !data->Command.empty() )
+    {
+    newLFF.Arguments.clear();
+    newLFF.Arguments.push_back(
+      cmListFileArgument(variable, true, "unknown", 9999));
+    newLFF.Arguments.push_back(
+      cmListFileArgument(accessString, true, "unknown", 9999));
+    newLFF.Arguments.push_back(
+      cmListFileArgument(newValue?newValue:"", true, "unknown", 9999));
+    newLFF.Arguments.push_back(
+      cmListFileArgument(currentListFile, true, "unknown", 9999));
+    newLFF.Arguments.push_back(
+      cmListFileArgument(stack, true, "unknown", 9999));
+    newLFF.Name = data->Command;
+    newLFF.FilePath = "Some weird path";
+    newLFF.Line = 9999;
+    cmExecutionStatus status;
+    if(!makefile->ExecuteCommand(newLFF,status))
+      {
+      arg.FilePath =  "Unknown";
+      arg.Line = 0;
+      cmOStringStream error;
+      error << "Error in cmake code at\n"
+        << arg.FilePath << ":" << arg.Line << ":\n"
+        << "A command failed during the invocation of callback \""
+        << data->Command << "\".";
+      cmSystemTools::Error(error.str().c_str());
+      data->InCallback = false;
+      return;
+      }
+    processed = true;
+    }
+  if ( !processed )
+    {
+    cmOStringStream msg;
+    msg << "Variable \"" << variable.c_str() << "\" was accessed using "
+        << accessString << " with value \"" << (newValue?newValue:"") << "\".";
+    makefile->IssueMessage(cmake::LOG, msg.str());
+    }
+
+  data->InCallback = false;
+}
+
+//----------------------------------------------------------------------------
+static void deleteVariableWatchCallbackData(void* client_data)
+{
+  cmVariableWatchCallbackData* data
+    = static_cast<cmVariableWatchCallbackData*>(client_data);
+  delete data;
 }
 
 //----------------------------------------------------------------------------
 cmVariableWatchCommand::cmVariableWatchCommand()
 {
-  this->InCallback = false;
+}
+
+//----------------------------------------------------------------------------
+cmVariableWatchCommand::~cmVariableWatchCommand()
+{
+  std::set<std::string>::const_iterator it;
+  for ( it = this->WatchedVariables.begin();
+        it != this->WatchedVariables.end();
+        ++it )
+    {
+    this->Makefile->GetCMakeInstance()->GetVariableWatch()->RemoveWatch(
+      *it, cmVariableWatchCommandVariableAccessed);
+    }
 }
 
 //----------------------------------------------------------------------------
@@ -39,10 +123,10 @@ bool cmVariableWatchCommand
     return false;
     }
   std::string variable = args[0];
+  std::string command;
   if ( args.size() > 1 )
     {
-    std::string command = args[1];
+    command = args[1];
-    this->Handlers[variable].Commands.push_back(args[1]);
     }
   if ( variable == "CMAKE_CURRENT_LIST_FILE" )
     {
@@ -52,74 +136,19 @@ bool cmVariableWatchCommand
     return false;
     }
 
-  this->Makefile->GetCMakeInstance()->GetVariableWatch()->AddWatch(
+  cmVariableWatchCallbackData* data = new cmVariableWatchCallbackData;
-    variable, cmVariableWatchCommandVariableAccessed, this);
+
+  data->InCallback = false;
+  data->Command = command;
+
+  this->WatchedVariables.insert(variable);
+  if ( !this->Makefile->GetCMakeInstance()->GetVariableWatch()->AddWatch(
+          variable, cmVariableWatchCommandVariableAccessed,
+          data, deleteVariableWatchCallbackData) )
+    {
+    deleteVariableWatchCallbackData(data);
+    return false;
+    }
 
   return true;
 }
-
-//----------------------------------------------------------------------------
-void cmVariableWatchCommand::VariableAccessed(const std::string& variable,
-  int access_type, const char* newValue, const cmMakefile* mf)
-{
-  if ( this->InCallback )
-    {
-    return;
-    }
-  this->InCallback = true;
-
-  cmListFileFunction newLFF;
-  cmVariableWatchCommandHandler *handler = &this->Handlers[variable];
-  cmVariableWatchCommandHandler::VectorOfCommands::iterator it;
-  cmListFileArgument arg;
-  bool processed = false;
-  const char* accessString = cmVariableWatch::GetAccessAsString(access_type);
-  const char* currentListFile = mf->GetDefinition("CMAKE_CURRENT_LIST_FILE");
-
-  /// Ultra bad!!
-  cmMakefile* makefile = const_cast<cmMakefile*>(mf);
-
-  std::string stack = makefile->GetProperty("LISTFILE_STACK");
-  for ( it = handler->Commands.begin(); it != handler->Commands.end();
-    ++ it )
-    {
-    std::string command = *it;
-    newLFF.Arguments.clear();
-    newLFF.Arguments.push_back(
-      cmListFileArgument(variable, true, "unknown", 9999));
-    newLFF.Arguments.push_back(
-      cmListFileArgument(accessString, true, "unknown", 9999));
-    newLFF.Arguments.push_back(
-      cmListFileArgument(newValue?newValue:"", true, "unknown", 9999));
-    newLFF.Arguments.push_back(
-      cmListFileArgument(currentListFile, true, "unknown", 9999));
-    newLFF.Arguments.push_back(
-      cmListFileArgument(stack, true, "unknown", 9999));
-    newLFF.Name = command;
-    newLFF.FilePath = "Some weird path";
-    newLFF.Line = 9999;
-    cmExecutionStatus status;
-    if(!makefile->ExecuteCommand(newLFF,status))
-      {
-      arg.FilePath =  "Unknown";
-      arg.Line = 0;
-      cmOStringStream error;
-      error << "Error in cmake code at\n"
-        << arg.FilePath << ":" << arg.Line << ":\n"
-        << "A command failed during the invocation of callback\""
-        << command << "\".";
-      cmSystemTools::Error(error.str().c_str());
-      this->InCallback = false;
-      return;
-      }
-    processed = true;
-    }
-  if ( !processed )
-    {
-    cmOStringStream msg;
-    msg << "Variable \"" << variable.c_str() << "\" was accessed using "
-        << accessString << " with value \"" << newValue << "\".";
-    makefile->IssueMessage(cmake::LOG, msg.str());
-    }
-  this->InCallback = false;
-}

Source/cmVariableWatchCommand.h

@@ -14,13 +14,6 @@
 
 #include "cmCommand.h"
 
-class cmVariableWatchCommandHandler
-{
-public:
-  typedef std::vector<std::string> VectorOfCommands;
-  VectorOfCommands Commands;
-};
-
 /** \class cmVariableWatchCommand
  * \brief Watch when the variable changes and invoke command
  *
@@ -39,6 +32,9 @@ public:
   //! Default constructor
   cmVariableWatchCommand();
 
+  //! Destructor.
+  ~cmVariableWatchCommand();
+
   /**
    * This is called when the command is first encountered in
    * the CMakeLists.txt file.
@@ -83,13 +79,8 @@ public:
 
   cmTypeMacro(cmVariableWatchCommand, cmCommand);
 
-  void VariableAccessed(const std::string& variable, int access_type,
-    const char* newValue, const cmMakefile* mf);
-
 protected:
-  std::map<std::string, cmVariableWatchCommandHandler> Handlers;
+  std::set<std::string> WatchedVariables;
-
-  bool InCallback;
 };
 
 

Tests/RunCMake/variable_watch/RunCMakeTest.cmake

@@ -2,3 +2,4 @@ include(RunCMake)
 
 run_cmake(ModifiedAccess)
 run_cmake(NoWatcher)
+run_cmake(WatchTwice)

Tests/RunCMake/variable_watch/WatchTwice-stderr.txt

@@ -0,0 +1,2 @@
+From watch1
+From watch2

Tests/RunCMake/variable_watch/WatchTwice.cmake

@@ -0,0 +1,11 @@
+function (watch1)
+    message("From watch1")
+endfunction ()
+
+function (watch2)
+    message("From watch2")
+endfunction ()
+
+variable_watch(watched watch1)
+variable_watch(watched watch2)
+set(access "${watched}")