From fc7c3b4dc8522ad489a6fb67ac6030f302c65df3 Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Fri, 2 Aug 2013 15:43:15 -0400 Subject: 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 3905e9b..21b910d 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 7dd4ac5..45273e5 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; -- cgit v0.12 From 0d6acb1df8c20bb21f4d328cf2c35d0cbb6d7ea3 Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Fri, 2 Aug 2013 15:44:15 -0400 Subject: 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 21b910d..21acd3b 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 45273e5..d666815 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; -- cgit v0.12 From e43e207c7bbde2a9e0948da0d1e79879ccd5ce45 Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Tue, 6 Aug 2013 14:08:57 -0400 Subject: 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 21acd3b..c2a899d 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 d666815..3b6cafd 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); -- cgit v0.12 From 34b397e8dec04c55b415ee0e5f9172f624156d36 Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Tue, 6 Aug 2013 14:12:54 -0400 Subject: 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 c2a899d..d049cdd 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 3b6cafd..790c75a 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 -- cgit v0.12 From 00ce12a3347d1fef25badff00c5abc3dc715f207 Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Fri, 2 Aug 2013 15:49:28 -0400 Subject: 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 d049cdd..8ad6fce 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 ) -- cgit v0.12 From 05dad99f5a1ee781da52df0ccc61b236087bcd2d Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Fri, 2 Aug 2013 15:49:50 -0400 Subject: 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 297a92b..2b81d79 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; -- cgit v0.12 From f9bb20fe2bf1d9154a3244579ea84400912473b4 Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Fri, 2 Aug 2013 15:41:45 -0400 Subject: 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 | 137 +++++++++++++++++++++++--------------- Source/cmVariableWatchCommand.h | 17 ++--- 2 files changed, 87 insertions(+), 67 deletions(-) diff --git a/Source/cmVariableWatchCommand.cxx b/Source/cmVariableWatchCommand.cxx index 2b81d79..3b75ade 100644 --- a/Source/cmVariableWatchCommand.cxx +++ b/Source/cmVariableWatchCommand.cxx @@ -14,63 +14,27 @@ #include "cmVariableWatch.h" //---------------------------------------------------------------------------- -static void cmVariableWatchCommandVariableAccessed( - const std::string& variable, int access_type, void* client_data, - const char* newValue, const cmMakefile* mf) +struct cmVariableWatchCallbackData { - cmVariableWatchCommand* command - = static_cast(client_data); - command->VariableAccessed(variable, access_type, newValue, mf); -} + bool InCallback; + std::string Command; +}; //---------------------------------------------------------------------------- -cmVariableWatchCommand::cmVariableWatchCommand() +static void cmVariableWatchCommandVariableAccessed( + const std::string& variable, int access_type, void* client_data, + const char* newValue, const cmMakefile* mf) { - this->InCallback = false; -} + cmVariableWatchCallbackData* data + = static_cast(client_data); -//---------------------------------------------------------------------------- -bool cmVariableWatchCommand -::InitialPass(std::vector const& args, cmExecutionStatus &) -{ - if ( args.size() < 1 ) - { - this->SetError("must be called with at least one argument."); - return false; - } - std::string variable = args[0]; - if ( args.size() > 1 ) - { - std::string command = args[1]; - this->Handlers[variable].Commands.push_back(args[1]); - } - if ( variable == "CMAKE_CURRENT_LIST_FILE" ) - { - cmOStringStream ostr; - ostr << "cannot be set on the variable: " << variable.c_str(); - this->SetError(ostr.str().c_str()); - return false; - } - - this->Makefile->GetCMakeInstance()->GetVariableWatch()->AddWatch( - variable, cmVariableWatchCommandVariableAccessed, this); - - return true; -} - -//---------------------------------------------------------------------------- -void cmVariableWatchCommand::VariableAccessed(const std::string& variable, - int access_type, const char* newValue, const cmMakefile* mf) -{ - if ( this->InCallback ) + if ( data->InCallback ) { return; } - this->InCallback = true; + data->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); @@ -80,10 +44,8 @@ void cmVariableWatchCommand::VariableAccessed(const std::string& variable, cmMakefile* makefile = const_cast(mf); std::string stack = makefile->GetProperty("LISTFILE_STACK"); - for ( it = handler->Commands.begin(); it != handler->Commands.end(); - ++ it ) + if ( !data->Command.empty() ) { - std::string command = *it; newLFF.Arguments.clear(); newLFF.Arguments.push_back( cmListFileArgument(variable, true, "unknown", 9999)); @@ -95,7 +57,7 @@ void cmVariableWatchCommand::VariableAccessed(const std::string& variable, cmListFileArgument(currentListFile, true, "unknown", 9999)); newLFF.Arguments.push_back( cmListFileArgument(stack, true, "unknown", 9999)); - newLFF.Name = command; + newLFF.Name = data->Command; newLFF.FilePath = "Some weird path"; newLFF.Line = 9999; cmExecutionStatus status; @@ -107,9 +69,9 @@ void cmVariableWatchCommand::VariableAccessed(const std::string& variable, error << "Error in cmake code at\n" << arg.FilePath << ":" << arg.Line << ":\n" << "A command failed during the invocation of callback \"" - << command << "\"."; + << data->Command << "\"."; cmSystemTools::Error(error.str().c_str()); - this->InCallback = false; + data->InCallback = false; return; } processed = true; @@ -121,5 +83,72 @@ void cmVariableWatchCommand::VariableAccessed(const std::string& variable, << accessString << " with value \"" << newValue << "\"."; makefile->IssueMessage(cmake::LOG, msg.str()); } - this->InCallback = false; + + data->InCallback = false; +} + +//---------------------------------------------------------------------------- +static void deleteVariableWatchCallbackData(void* client_data) +{ + cmVariableWatchCallbackData* data + = static_cast(client_data); + delete data; +} + +//---------------------------------------------------------------------------- +cmVariableWatchCommand::cmVariableWatchCommand() +{ +} + +//---------------------------------------------------------------------------- +cmVariableWatchCommand::~cmVariableWatchCommand() +{ + std::set::const_iterator it; + for ( it = this->WatchedVariables.begin(); + it != this->WatchedVariables.end(); + ++it ) + { + this->Makefile->GetCMakeInstance()->GetVariableWatch()->RemoveWatch( + *it, cmVariableWatchCommandVariableAccessed); + } +} + +//---------------------------------------------------------------------------- +bool cmVariableWatchCommand +::InitialPass(std::vector const& args, cmExecutionStatus &) +{ + if ( args.size() < 1 ) + { + this->SetError("must be called with at least one argument."); + return false; + } + std::string variable = args[0]; + std::string command; + if ( args.size() > 1 ) + { + command = args[1]; + } + if ( variable == "CMAKE_CURRENT_LIST_FILE" ) + { + cmOStringStream ostr; + ostr << "cannot be set on the variable: " << variable.c_str(); + this->SetError(ostr.str().c_str()); + return false; + } + + 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; } diff --git a/Source/cmVariableWatchCommand.h b/Source/cmVariableWatchCommand.h index 3abc088..545535c 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; }; -- cgit v0.12 From b86e37c84f9691cc122e838b653ae0f441c4d1e2 Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Tue, 6 Aug 2013 14:32:19 -0400 Subject: 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 3b75ade..4eff19e 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()); } -- cgit v0.12 From 6aa0c214054252d1fc94e5b8e6b6ffc3eae4c08b Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Tue, 6 Aug 2013 14:31:28 -0400 Subject: 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 8d20476..9becb4c 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 0000000..fa7d347 --- /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 0000000..540cfc1 --- /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}") -- cgit v0.12