From fc7c3b4dc8522ad489a6fb67ac6030f302c65df3 Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Fri, 2 Aug 2013 15:43:15 -0400 Subject: [PATCH 1/9] variable_watch: Store client data as pointers The STL containers create extra copies which makes keeping track of the owner of the client data much messier. --- Source/cmVariableWatch.cxx | 27 +++++++++++++++++++++------ Source/cmVariableWatch.h | 2 +- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/Source/cmVariableWatch.cxx b/Source/cmVariableWatch.cxx index 3905e9bda..21b910dea 100644 --- a/Source/cmVariableWatch.cxx +++ b/Source/cmVariableWatch.cxx @@ -37,21 +37,35 @@ 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, WatchMethod method, void* client_data /*=0*/) { - cmVariableWatch::Pair p; - p.Method = method; - p.ClientData = client_data; + cmVariableWatch::Pair* p = new cmVariableWatch::Pair; + p->Method = method; + p->ClientData = client_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 ) { + delete pair; (*vp)[cc] = p; return; } @@ -66,8 +80,9 @@ void cmVariableWatch::RemoveWatch(const std::string& variable, cmVariableWatch::VectorOfPairs::iterator it; for ( it = vp->begin(); it != vp->end(); ++it ) { - if ( it->Method == method ) + if ( (*it)->Method == method ) { + delete *it; vp->erase(it); return; } @@ -87,7 +102,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); } } diff --git a/Source/cmVariableWatch.h b/Source/cmVariableWatch.h index 7dd4ac5e6..45273e5ae 100644 --- a/Source/cmVariableWatch.h +++ b/Source/cmVariableWatch.h @@ -70,7 +70,7 @@ protected: Pair() : Method(0), ClientData(0) {} }; - typedef std::vector< Pair > VectorOfPairs; + typedef std::vector< Pair* > VectorOfPairs; typedef std::map StringToVectorOfPairs; StringToVectorOfPairs WatchMap; From 0d6acb1df8c20bb21f4d328cf2c35d0cbb6d7ea3 Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Fri, 2 Aug 2013 15:44:15 -0400 Subject: [PATCH 2/9] variable_watch: Add a deleter for the client data The client data is arbitrary and the callback may be called an unspecified number of times, so the cmVariableWatch must be the one to delete the client data in the end (if it is needed at all). --- Source/cmVariableWatch.cxx | 4 +++- Source/cmVariableWatch.h | 13 +++++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/Source/cmVariableWatch.cxx b/Source/cmVariableWatch.cxx index 21b910dea..21acd3b7e 100644 --- a/Source/cmVariableWatch.cxx +++ b/Source/cmVariableWatch.cxx @@ -53,11 +53,13 @@ cmVariableWatch::~cmVariableWatch() } void 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 = new cmVariableWatch::Pair; p->Method = method; 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 ++ ) diff --git a/Source/cmVariableWatch.h b/Source/cmVariableWatch.h index 45273e5ae..d66681598 100644 --- a/Source/cmVariableWatch.h +++ b/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(); @@ -34,7 +35,7 @@ public: * Add watch to the variable */ void 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); /** @@ -67,7 +68,15 @@ 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; From e43e207c7bbde2a9e0948da0d1e79879ccd5ce45 Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Tue, 6 Aug 2013 14:08:57 -0400 Subject: [PATCH 3/9] variable_watch: Match client_data when finding duplicates If a callback has the same data as another call, we don't want to delete the old callback. This is because if the client_data is the same, it might get deleted causing the new client_data to be bogus. Now, AddWatch will return true if it will use the watch, false otherwise. Callers should check the return value to know whether client_data was adopted by the watch or not. --- Source/cmVariableWatch.cxx | 11 ++++++----- Source/cmVariableWatch.h | 2 +- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/Source/cmVariableWatch.cxx b/Source/cmVariableWatch.cxx index 21acd3b7e..c2a899dcd 100644 --- a/Source/cmVariableWatch.cxx +++ b/Source/cmVariableWatch.cxx @@ -52,7 +52,7 @@ cmVariableWatch::~cmVariableWatch() } } -void cmVariableWatch::AddWatch(const std::string& variable, +bool cmVariableWatch::AddWatch(const std::string& variable, WatchMethod method, void* client_data /*=0*/, DeleteData delete_data /*=0*/) { @@ -65,14 +65,15 @@ void cmVariableWatch::AddWatch(const std::string& variable, for ( cc = 0; cc < vp->size(); cc ++ ) { cmVariableWatch::Pair* pair = (*vp)[cc]; - if ( pair->Method == method ) + if ( pair->Method == method && + client_data && client_data == pair->ClientData) { - delete pair; - (*vp)[cc] = p; - return; + // Callback already exists + return false; } } vp->push_back(p); + return true; } void cmVariableWatch::RemoveWatch(const std::string& variable, diff --git a/Source/cmVariableWatch.h b/Source/cmVariableWatch.h index d66681598..3b6cafdb3 100644 --- a/Source/cmVariableWatch.h +++ b/Source/cmVariableWatch.h @@ -34,7 +34,7 @@ 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, DeleteData delete_data=0); void RemoveWatch(const std::string& variable, WatchMethod method); From 34b397e8dec04c55b415ee0e5f9172f624156d36 Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Tue, 6 Aug 2013 14:12:54 -0400 Subject: [PATCH 4/9] variable_watch: Allow specifying the data to match in RemoveWatch Now that watches are dependent on their client_data when adding, it also makes sense to allow matching the data for removal. --- Source/cmVariableWatch.cxx | 8 ++++++-- Source/cmVariableWatch.h | 3 ++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/Source/cmVariableWatch.cxx b/Source/cmVariableWatch.cxx index c2a899dcd..d049cddea 100644 --- a/Source/cmVariableWatch.cxx +++ b/Source/cmVariableWatch.cxx @@ -77,13 +77,17 @@ bool cmVariableWatch::AddWatch(const std::string& variable, } void cmVariableWatch::RemoveWatch(const std::string& variable, - WatchMethod method) + WatchMethod method, + void* client_data /*=0*/) { 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); diff --git a/Source/cmVariableWatch.h b/Source/cmVariableWatch.h index 3b6cafdb3..790c75acc 100644 --- a/Source/cmVariableWatch.h +++ b/Source/cmVariableWatch.h @@ -36,7 +36,8 @@ public: */ bool AddWatch(const std::string& variable, WatchMethod method, 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 From 00ce12a3347d1fef25badff00c5abc3dc715f207 Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Fri, 2 Aug 2013 15:49:28 -0400 Subject: [PATCH 5/9] variable_watch: Prevent making extra entries in the watch map When removing a watch on a variable, using the operator [] on the internal map will create an empty watch if the variable doesn't have any existing watches. Rather than creating this empty structure in the map, return if there isn't a watch on the variable already. --- Source/cmVariableWatch.cxx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Source/cmVariableWatch.cxx b/Source/cmVariableWatch.cxx index d049cddea..8ad6fce8f 100644 --- a/Source/cmVariableWatch.cxx +++ b/Source/cmVariableWatch.cxx @@ -80,6 +80,10 @@ void cmVariableWatch::RemoveWatch(const std::string& variable, 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 ) From 05dad99f5a1ee781da52df0ccc61b236087bcd2d Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Fri, 2 Aug 2013 15:49:50 -0400 Subject: [PATCH 6/9] variable_watch: Fix a typo in the error message There was no space between "callback" and the quoted command name. --- Source/cmVariableWatchCommand.cxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/cmVariableWatchCommand.cxx b/Source/cmVariableWatchCommand.cxx index 297a92b93..2b81d79e2 100644 --- a/Source/cmVariableWatchCommand.cxx +++ b/Source/cmVariableWatchCommand.cxx @@ -106,7 +106,7 @@ void cmVariableWatchCommand::VariableAccessed(const std::string& variable, cmOStringStream error; error << "Error in cmake code at\n" << arg.FilePath << ":" << arg.Line << ":\n" - << "A command failed during the invocation of callback\"" + << "A command failed during the invocation of callback \"" << command << "\"."; cmSystemTools::Error(error.str().c_str()); this->InCallback = false; From f9bb20fe2bf1d9154a3244579ea84400912473b4 Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Fri, 2 Aug 2013 15:41:45 -0400 Subject: [PATCH 7/9] variable_watch: Don't share memory for callbacks The command itself is owned by the cmMakefile class, but the cmVariableWatch which holds a pointer to the cmVariableWatchCommand via the client_data for the callback outlives the cmMakefile class in the Qt GUI. This means that when the cmMakefile is destroyed, the variable watch is still in effect, but with a stale pointer. To fix this, each callback is now a separate entity completely and doesn't rely on the command which spawned it at all. An example CMakeLists.txt which demonstrates the issue (only displayed in cmake-gui, so no tests can be written for it): set(var 0) variable_watch(var) --- Source/cmVariableWatchCommand.cxx | 177 +++++++++++++++++------------- Source/cmVariableWatchCommand.h | 17 +-- 2 files changed, 107 insertions(+), 87 deletions(-) diff --git a/Source/cmVariableWatchCommand.cxx b/Source/cmVariableWatchCommand.cxx index 2b81d79e2..3b75ade44 100644 --- a/Source/cmVariableWatchCommand.cxx +++ b/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 - = static_cast(client_data); - command->VariableAccessed(variable, access_type, newValue, mf); + cmVariableWatchCallbackData* data + = static_cast(client_data); + + 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(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 << "\"."; + makefile->IssueMessage(cmake::LOG, msg.str()); + } + + data->InCallback = false; +} + +//---------------------------------------------------------------------------- +static void deleteVariableWatchCallbackData(void* client_data) +{ + cmVariableWatchCallbackData* data + = static_cast(client_data); + delete data; } //---------------------------------------------------------------------------- cmVariableWatchCommand::cmVariableWatchCommand() { - this->InCallback = false; +} + +//---------------------------------------------------------------------------- +cmVariableWatchCommand::~cmVariableWatchCommand() +{ + std::set::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]; - this->Handlers[variable].Commands.push_back(args[1]); + command = args[1]; } if ( variable == "CMAKE_CURRENT_LIST_FILE" ) { @@ -52,74 +136,19 @@ bool cmVariableWatchCommand return false; } - this->Makefile->GetCMakeInstance()->GetVariableWatch()->AddWatch( - variable, cmVariableWatchCommandVariableAccessed, this); + cmVariableWatchCallbackData* data = new cmVariableWatchCallbackData; + + 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(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; -} diff --git a/Source/cmVariableWatchCommand.h b/Source/cmVariableWatchCommand.h index 3abc08894..545535c40 100644 --- a/Source/cmVariableWatchCommand.h +++ b/Source/cmVariableWatchCommand.h @@ -14,13 +14,6 @@ #include "cmCommand.h" -class cmVariableWatchCommandHandler -{ -public: - typedef std::vector 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 Handlers; - - bool InCallback; + std::set WatchedVariables; }; From b86e37c84f9691cc122e838b653ae0f441c4d1e2 Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Tue, 6 Aug 2013 14:32:19 -0400 Subject: [PATCH 8/9] variable_watch: Check newValue for NULL On read access, newValue can be NULL since there is no new value, so use the empty string instead. --- Source/cmVariableWatchCommand.cxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/cmVariableWatchCommand.cxx b/Source/cmVariableWatchCommand.cxx index 3b75ade44..4eff19e22 100644 --- a/Source/cmVariableWatchCommand.cxx +++ b/Source/cmVariableWatchCommand.cxx @@ -80,7 +80,7 @@ static void cmVariableWatchCommandVariableAccessed( { cmOStringStream msg; msg << "Variable \"" << variable.c_str() << "\" was accessed using " - << accessString << " with value \"" << newValue << "\"."; + << accessString << " with value \"" << (newValue?newValue:"") << "\"."; makefile->IssueMessage(cmake::LOG, msg.str()); } From 6aa0c214054252d1fc94e5b8e6b6ffc3eae4c08b Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Tue, 6 Aug 2013 14:31:28 -0400 Subject: [PATCH 9/9] variable_watch: Add test for watching a variable multiple times --- Tests/RunCMake/variable_watch/RunCMakeTest.cmake | 1 + Tests/RunCMake/variable_watch/WatchTwice-stderr.txt | 2 ++ Tests/RunCMake/variable_watch/WatchTwice.cmake | 11 +++++++++++ 3 files changed, 14 insertions(+) create mode 100644 Tests/RunCMake/variable_watch/WatchTwice-stderr.txt create mode 100644 Tests/RunCMake/variable_watch/WatchTwice.cmake diff --git a/Tests/RunCMake/variable_watch/RunCMakeTest.cmake b/Tests/RunCMake/variable_watch/RunCMakeTest.cmake index 8d2047673..9becb4cd7 100644 --- a/Tests/RunCMake/variable_watch/RunCMakeTest.cmake +++ b/Tests/RunCMake/variable_watch/RunCMakeTest.cmake @@ -2,3 +2,4 @@ include(RunCMake) run_cmake(ModifiedAccess) run_cmake(NoWatcher) +run_cmake(WatchTwice) diff --git a/Tests/RunCMake/variable_watch/WatchTwice-stderr.txt b/Tests/RunCMake/variable_watch/WatchTwice-stderr.txt new file mode 100644 index 000000000..fa7d3472a --- /dev/null +++ b/Tests/RunCMake/variable_watch/WatchTwice-stderr.txt @@ -0,0 +1,2 @@ +From watch1 +From watch2 diff --git a/Tests/RunCMake/variable_watch/WatchTwice.cmake b/Tests/RunCMake/variable_watch/WatchTwice.cmake new file mode 100644 index 000000000..540cfc160 --- /dev/null +++ b/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}")