From 33386f4c5f2576e87a0fc884710a00dac892a21c Mon Sep 17 00:00:00 2001 From: masawei Date: Mon, 11 Aug 2014 00:02:13 +0200 Subject: [PATCH] Changed the actions in the filters to be shared_ptr instead of raw pointers. This prevents memory leaks when a filter is destructed. - Also handled nullptr actions. |- They are checked for in the constructor as well as in the add method and filtered out. No segfaults do to nullptr actions anymore. Former-commit-id: 84b3b2a978509f13dc62763d05058b3653ba482b --- src/formula/AbstractFilter.h | 52 ++++++++++++++++++---- src/formula/csl/CslFilter.h | 4 +- src/formula/ltl/LtlFilter.h | 4 +- src/formula/prctl/PrctlFilter.h | 6 +-- src/parser/CslParser.cpp | 36 +++++++-------- src/parser/LtlParser.cpp | 32 ++++++------- src/parser/PrctlParser.cpp | 28 ++++++------ test/functional/parser/PrctlParserTest.cpp | 10 ++--- 8 files changed, 104 insertions(+), 68 deletions(-) diff --git a/src/formula/AbstractFilter.h b/src/formula/AbstractFilter.h index d0f9aa60e..59bc935fa 100644 --- a/src/formula/AbstractFilter.h +++ b/src/formula/AbstractFilter.h @@ -30,16 +30,42 @@ public: // Intentionally left empty. } - AbstractFilter(action::AbstractAction* action, OptimizingOperator opt = UNDEFINED) : opt(opt) { - actions.push_back(action); + AbstractFilter(std::shared_ptr> const & action, OptimizingOperator opt = UNDEFINED) : opt(opt) { + if(action.get() != nullptr) { + actions.push_back(action); + } } - AbstractFilter(std::vector*> actions, OptimizingOperator opt = UNDEFINED) : actions(actions), opt(opt) { - // Intentionally left empty. + AbstractFilter(std::vector>> const & actions, OptimizingOperator opt = UNDEFINED) { + // Filter out all nullptr actions. + // First detect that there is at least one. + uint_fast64_t emptyCount = 0; + for(uint_fast64_t i = 0; i < actions.size(); i++) { + if (actions[i].get() == nullptr) { + emptyCount++; + } + } + + if(emptyCount > 0) { + // There is at least one nullptr action. + // Allocate space for the non null actions. + this->actions = std::vector>>(actions.size() - emptyCount); + + // Fill the vector. Note: For most implementations of the standard there will be no reallocation in the vector while doing this. + for(auto action : actions){ + if(action.get() != nullptr) { + this->actions.push_back(action); + } + } + } else { + this->actions = actions; + } + + this->opt = opt; } virtual ~AbstractFilter() { - actions.clear(); + // Intentionally left empty. } virtual std::string toString() const { @@ -70,8 +96,8 @@ public: return desc; } - void addAction(action::AbstractAction* action) { - if(action != nullptr) { + void addAction(std::shared_ptr> const & action) { + if(action.get() != nullptr) { actions.push_back(action); } } @@ -80,6 +106,16 @@ public: actions.pop_back(); } + std::shared_ptr> getAction(uint_fast64_t position) { + // Make sure the chosen position is not beyond the end of the vector. + // If it is so return the last element. + if(position < actions.size()) { + return actions[position]; + } else { + return actions[actions.size()-1]; + } + } + uint_fast64_t getActionCount() const { return actions.size(); } @@ -94,7 +130,7 @@ public: protected: - std::vector*> actions; + std::vector>> actions; OptimizingOperator opt; }; diff --git a/src/formula/csl/CslFilter.h b/src/formula/csl/CslFilter.h index b7c99df39..fffd78663 100644 --- a/src/formula/csl/CslFilter.h +++ b/src/formula/csl/CslFilter.h @@ -43,11 +43,11 @@ public: // Intentionally left empty. } - CslFilter(std::shared_ptr> const & child, action::AbstractAction* action, OptimizingOperator opt = UNDEFINED, bool steadyStateQuery = false) : AbstractFilter(action, opt), child(child), steadyStateQuery(steadyStateQuery) { + CslFilter(std::shared_ptr> const & child, std::shared_ptr> const & action, OptimizingOperator opt = UNDEFINED, bool steadyStateQuery = false) : AbstractFilter(action, opt), child(child), steadyStateQuery(steadyStateQuery) { // Intentionally left empty } - CslFilter(std::shared_ptr> const & child, std::vector*> actions, OptimizingOperator opt = UNDEFINED, bool steadyStateQuery = false) : AbstractFilter(actions, opt), child(child), steadyStateQuery(steadyStateQuery) { + CslFilter(std::shared_ptr> const & child, std::vector>> const & actions, OptimizingOperator opt = UNDEFINED, bool steadyStateQuery = false) : AbstractFilter(actions, opt), child(child), steadyStateQuery(steadyStateQuery) { // Intentionally left empty. } diff --git a/src/formula/ltl/LtlFilter.h b/src/formula/ltl/LtlFilter.h index a13148b86..d35b9fd46 100644 --- a/src/formula/ltl/LtlFilter.h +++ b/src/formula/ltl/LtlFilter.h @@ -41,11 +41,11 @@ public: // Intentionally left empty. } - LtlFilter(std::shared_ptr> const & child, action::AbstractAction* action, OptimizingOperator opt = UNDEFINED) : AbstractFilter(action, opt), child(child) { + LtlFilter(std::shared_ptr> const & child, std::shared_ptr> const & action, OptimizingOperator opt = UNDEFINED) : AbstractFilter(action, opt), child(child) { this->actions.push_back(action); } - LtlFilter(std::shared_ptr> const & child, std::vector*> actions, OptimizingOperator opt = UNDEFINED) : AbstractFilter(actions, opt), child(child) { + LtlFilter(std::shared_ptr> const & child, std::vector>> const & actions, OptimizingOperator opt = UNDEFINED) : AbstractFilter(actions, opt), child(child) { // Intentionally left empty. } diff --git a/src/formula/prctl/PrctlFilter.h b/src/formula/prctl/PrctlFilter.h index 161a9d3f8..48967486e 100644 --- a/src/formula/prctl/PrctlFilter.h +++ b/src/formula/prctl/PrctlFilter.h @@ -46,16 +46,16 @@ public: // Intentionally left empty. } - PrctlFilter(std::shared_ptr> const & child, action::AbstractAction* action, OptimizingOperator opt = UNDEFINED) : AbstractFilter(action, opt), child(child) { + PrctlFilter(std::shared_ptr> const & child, std::shared_ptr> const & action, OptimizingOperator opt = UNDEFINED) : AbstractFilter(action, opt), child(child) { // Intentionally left empty. } - PrctlFilter(std::shared_ptr> const & child, std::vector*> actions, OptimizingOperator opt = UNDEFINED) : AbstractFilter(actions, opt), child(child) { + PrctlFilter(std::shared_ptr> const & child, std::vector>> const & actions, OptimizingOperator opt = UNDEFINED) : AbstractFilter(actions, opt), child(child) { // Intentionally left empty. } virtual ~PrctlFilter() { - this->actions.clear(); + // Intentionally left empty. } void check(storm::modelchecker::prctl::AbstractModelChecker const & modelchecker) const { diff --git a/src/parser/CslParser.cpp b/src/parser/CslParser.cpp index c28d31b0a..f0a51afa6 100644 --- a/src/parser/CslParser.cpp +++ b/src/parser/CslParser.cpp @@ -148,35 +148,35 @@ struct CslParser::CslGrammar : qi::grammar qi::lit("(") >> comparisonType >> qi::lit(",") >> qi::double_ >> qi::lit(")"))[qi::_val = - phoenix::new_>(qi::_1, qi::_2)]; + MAKE(storm::property::action::BoundAction ,qi::_1, qi::_2)]; boundAction.name("bound action"); - invertAction = qi::lit("invert")[qi::_val = phoenix::new_>()]; + invertAction = qi::lit("invert")[qi::_val = MAKE(storm::property::action::InvertAction, )]; invertAction.name("invert action"); formulaAction = (qi::lit("formula") > qi::lit("(") >> stateFormula >> qi::lit(")"))[qi::_val = - phoenix::new_>(qi::_1)]; + MAKE(storm::property::action::FormulaAction, qi::_1)]; formulaAction.name("formula action"); rangeAction = ( (qi::lit("range") >> qi::lit("(") >> qi::uint_ >> qi::lit(",") > qi::uint_ >> qi::lit(")"))[qi::_val = - phoenix::new_>(qi::_1, qi::_2)] | + MAKE(storm::property::action::RangeAction, qi::_1, qi::_2)] | (qi::lit("range") >> qi::lit("(") >> qi::uint_ >> qi::lit(")"))[qi::_val = - phoenix::new_>(qi::_1, qi::_1 + 1)] + MAKE(storm::property::action::RangeAction, qi::_1, qi::_1 + 1)] ); rangeAction.name("range action"); sortAction = ( - (qi::lit("sort") > qi::lit("(") >> sortingCategory >> qi::lit(")"))[qi::_val = - phoenix::new_>(qi::_1)] | - (qi::lit("sort") > qi::lit("(") >> sortingCategory >> qi::lit(", ") >> qi::lit("asc") > qi::lit(")"))[qi::_val = - phoenix::new_>(qi::_1, true)] | - (qi::lit("sort") > qi::lit("(") >> sortingCategory >> qi::lit(", ") >> qi::lit("desc") > qi::lit(")"))[qi::_val = - phoenix::new_>(qi::_1, false)] + (qi::lit("sort") >> qi::lit("(") >> sortingCategory >> qi::lit(")"))[qi::_val = + MAKE(storm::property::action::SortAction, qi::_1)] | + (qi::lit("sort") >> qi::lit("(") >> sortingCategory >> qi::lit(", ") >> (qi::lit("ascending") | qi::lit("asc")) > qi::lit(")"))[qi::_val = + MAKE(storm::property::action::SortAction, qi::_1, true)] | + (qi::lit("sort") >> qi::lit("(") >> sortingCategory >> qi::lit(", ") >> (qi::lit("descending") | qi::lit("desc")) > qi::lit(")"))[qi::_val = + MAKE(storm::property::action::SortAction, qi::_1, false)] ); sortAction.name("sort action"); - abstractAction = (boundAction | invertAction | formulaAction | rangeAction | sortAction) >> (qi::eps | qi::lit(";")); + abstractAction = (boundAction | invertAction | formulaAction | rangeAction | sortAction) >> (qi::lit(";") | qi::eps); abstractAction.name("filter action"); filter = (qi::lit("filter") >> qi::lit("[") >> +abstractAction >> qi::lit("]") >> qi::lit("(") >> formula >> qi::lit(")"))[qi::_val = @@ -200,12 +200,12 @@ struct CslParser::CslGrammar : qi::grammar>(), Skipper> probabilisticNoBoundOperator; qi::rule>(), Skipper> steadyStateNoBoundOperator; - qi::rule*(), Skipper> abstractAction; - qi::rule*(), Skipper> boundAction; - qi::rule*(), Skipper> invertAction; - qi::rule*(), Skipper> formulaAction; - qi::rule*(), Skipper> rangeAction; - qi::rule*(), Skipper> sortAction; + qi::rule>(), Skipper> abstractAction; + qi::rule>(), Skipper> boundAction; + qi::rule>(), Skipper> invertAction; + qi::rule>(), Skipper> formulaAction; + qi::rule>(), Skipper> rangeAction; + qi::rule>(), Skipper> sortAction; qi::rule>(), Skipper> formula; qi::rule>(), Skipper> comment; diff --git a/src/parser/LtlParser.cpp b/src/parser/LtlParser.cpp index 3bc614243..25d62d241 100644 --- a/src/parser/LtlParser.cpp +++ b/src/parser/LtlParser.cpp @@ -109,31 +109,31 @@ struct LtlParser::LtlGrammar : qi::grammar qi::lit("(") >> comparisonType >> qi::lit(",") >> qi::double_ >> qi::lit(")"))[qi::_val = - phoenix::new_>(qi::_1, qi::_2)]; + MAKE(storm::property::action::BoundAction ,qi::_1, qi::_2)]; boundAction.name("bound action"); - invertAction = qi::lit("invert")[qi::_val = phoenix::new_>()]; + invertAction = qi::lit("invert")[qi::_val = MAKE(storm::property::action::InvertAction, )]; invertAction.name("invert action"); rangeAction = ( (qi::lit("range") >> qi::lit("(") >> qi::uint_ >> qi::lit(",") > qi::uint_ >> qi::lit(")"))[qi::_val = - phoenix::new_>(qi::_1, qi::_2)] | + MAKE(storm::property::action::RangeAction, qi::_1, qi::_2)] | (qi::lit("range") >> qi::lit("(") >> qi::uint_ >> qi::lit(")"))[qi::_val = - phoenix::new_>(qi::_1, qi::_1 + 1)] + MAKE(storm::property::action::RangeAction, qi::_1, qi::_1 + 1)] ); rangeAction.name("range action"); sortAction = ( - (qi::lit("sort") > qi::lit("(") >> sortingCategory >> qi::lit(")"))[qi::_val = - phoenix::new_>(qi::_1)] | - (qi::lit("sort") > qi::lit("(") >> sortingCategory >> qi::lit(", ") >> qi::lit("asc") > qi::lit(")"))[qi::_val = - phoenix::new_>(qi::_1, true)] | - (qi::lit("sort") > qi::lit("(") >> sortingCategory >> qi::lit(", ") >> qi::lit("desc") > qi::lit(")"))[qi::_val = - phoenix::new_>(qi::_1, false)] + (qi::lit("sort") >> qi::lit("(") >> sortingCategory >> qi::lit(")"))[qi::_val = + MAKE(storm::property::action::SortAction, qi::_1)] | + (qi::lit("sort") >> qi::lit("(") >> sortingCategory >> qi::lit(", ") >> (qi::lit("ascending") | qi::lit("asc")) > qi::lit(")"))[qi::_val = + MAKE(storm::property::action::SortAction, qi::_1, true)] | + (qi::lit("sort") >> qi::lit("(") >> sortingCategory >> qi::lit(", ") >> (qi::lit("descending") | qi::lit("desc")) > qi::lit(")"))[qi::_val = + MAKE(storm::property::action::SortAction, qi::_1, false)] ); sortAction.name("sort action"); - abstractAction = (boundAction | invertAction | rangeAction | sortAction) >> (qi::eps | qi::lit(";")); + abstractAction = (boundAction | invertAction | rangeAction | sortAction) >> (qi::lit(";") | qi::eps); abstractAction.name("filter action"); filter = (qi::lit("filter") >> qi::lit("[") >> +abstractAction >> qi::lit("]") > qi::lit("(") >> formula >> qi::lit(")"))[qi::_val = @@ -153,11 +153,11 @@ struct LtlParser::LtlGrammar : qi::grammar>(), Skipper> start; qi::rule>(), Skipper> filter; - qi::rule*(), Skipper> abstractAction; - qi::rule*(), Skipper> boundAction; - qi::rule*(), Skipper> invertAction; - qi::rule*(), Skipper> rangeAction; - qi::rule*(), Skipper> sortAction; + qi::rule>(), Skipper> abstractAction; + qi::rule>(), Skipper> boundAction; + qi::rule>(), Skipper> invertAction; + qi::rule>(), Skipper> rangeAction; + qi::rule>(), Skipper> sortAction; qi::rule>(), Skipper> comment; qi::rule>(), Skipper> formula; diff --git a/src/parser/PrctlParser.cpp b/src/parser/PrctlParser.cpp index 35036ffcf..b31b621bb 100644 --- a/src/parser/PrctlParser.cpp +++ b/src/parser/PrctlParser.cpp @@ -155,31 +155,31 @@ struct PrctlParser::PrctlGrammar : qi::grammar qi::lit("(") >> comparisonType >> qi::lit(",") >> qi::double_ >> qi::lit(")"))[qi::_val = - phoenix::new_>(qi::_1, qi::_2)]; + MAKE(storm::property::action::BoundAction ,qi::_1, qi::_2)]; boundAction.name("bound action"); - invertAction = qi::lit("invert")[qi::_val = phoenix::new_>()]; + invertAction = qi::lit("invert")[qi::_val = MAKE(storm::property::action::InvertAction, )]; invertAction.name("invert action"); formulaAction = (qi::lit("formula") > qi::lit("(") >> stateFormula >> qi::lit(")"))[qi::_val = - phoenix::new_>(qi::_1)]; + MAKE(storm::property::action::FormulaAction, qi::_1)]; formulaAction.name("formula action"); rangeAction = ( (qi::lit("range") >> qi::lit("(") >> qi::uint_ >> qi::lit(",") > qi::uint_ >> qi::lit(")"))[qi::_val = - phoenix::new_>(qi::_1, qi::_2)] | + MAKE(storm::property::action::RangeAction, qi::_1, qi::_2)] | (qi::lit("range") >> qi::lit("(") >> qi::uint_ >> qi::lit(")"))[qi::_val = - phoenix::new_>(qi::_1, qi::_1 + 1)] + MAKE(storm::property::action::RangeAction, qi::_1, qi::_1 + 1)] ); rangeAction.name("range action"); sortAction = ( (qi::lit("sort") >> qi::lit("(") >> sortingCategory >> qi::lit(")"))[qi::_val = - phoenix::new_>(qi::_1)] | + MAKE(storm::property::action::SortAction, qi::_1)] | (qi::lit("sort") >> qi::lit("(") >> sortingCategory >> qi::lit(", ") >> (qi::lit("ascending") | qi::lit("asc")) > qi::lit(")"))[qi::_val = - phoenix::new_>(qi::_1, true)] | + MAKE(storm::property::action::SortAction, qi::_1, true)] | (qi::lit("sort") >> qi::lit("(") >> sortingCategory >> qi::lit(", ") >> (qi::lit("descending") | qi::lit("desc")) > qi::lit(")"))[qi::_val = - phoenix::new_>(qi::_1, false)] + MAKE(storm::property::action::SortAction, qi::_1, false)] ); sortAction.name("sort action"); @@ -210,12 +210,12 @@ struct PrctlParser::PrctlGrammar : qi::grammar>(), Skipper> probabilisticNoBoundOperator; qi::rule>(), Skipper> rewardNoBoundOperator; - qi::rule*(), Skipper> abstractAction; - qi::rule*(), Skipper> boundAction; - qi::rule*(), Skipper> invertAction; - qi::rule*(), Skipper> formulaAction; - qi::rule*(), Skipper> rangeAction; - qi::rule*(), Skipper> sortAction; + qi::rule>(), Skipper> abstractAction; + qi::rule>(), Skipper> boundAction; + qi::rule>(), Skipper> invertAction; + qi::rule>(), Skipper> formulaAction; + qi::rule>(), Skipper> rangeAction; + qi::rule>(), Skipper> sortAction; qi::rule>(), Skipper> formula; qi::rule>(), Skipper> comment; diff --git a/test/functional/parser/PrctlParserTest.cpp b/test/functional/parser/PrctlParserTest.cpp index 7f482f63e..ee86d92e8 100644 --- a/test/functional/parser/PrctlParserTest.cpp +++ b/test/functional/parser/PrctlParserTest.cpp @@ -169,11 +169,11 @@ TEST(PrctlParserTest, parsePrctlFilterTest) { ASSERT_NE(formula, nullptr); ASSERT_EQ(5, formula->getActionCount()); - ASSERT_NE(dynamic_cast*>(formula->getAction(0)), nullptr); - ASSERT_NE(dynamic_cast*>(formula->getAction(1)), nullptr); - ASSERT_NE(dynamic_cast*>(formula->getAction(2)), nullptr); - ASSERT_NE(dynamic_cast*>(formula->getAction(3)), nullptr); - ASSERT_NE(dynamic_cast*>(formula->getAction(4)), nullptr); + ASSERT_NE(std::dynamic_pointer_cast>(formula->getAction(0)).get(), nullptr); + ASSERT_NE(std::dynamic_pointer_cast>(formula->getAction(1)).get(), nullptr); + ASSERT_NE(std::dynamic_pointer_cast>(formula->getAction(2)).get(), nullptr); + ASSERT_NE(std::dynamic_pointer_cast>(formula->getAction(3)).get(), nullptr); + ASSERT_NE(std::dynamic_pointer_cast>(formula->getAction(4)).get(), nullptr); // The input was parsed correctly. ASSERT_EQ("filter[formula(b); invert; bound(<, 0.500000); sort(value, ascending); range(0, 3)](F a)", formula->toString());