integrated feedback on code quality by TQ

CHANGELOG.md

@@ -16,7 +16,7 @@ Version 1.6.x
 - API: Simulation of prism-models 
 - API: Model-builder takes a callback function to prevent extension of particular actions, prism-to-explicit mapping can be exported
 - API: Export of dice-formatted expressions
-- Prism-language/explicit builder: Allow action names in commands writing to global variables if these clearly do not write to actions. 
+- Prism-language/explicit builder: Allow action names in commands writing to global variables if these (clearly) do not conflict with assignments of synchronizing commads.
 - Prism-language: n-ary predicates are supported (e.g., ExactlyOneOf)
 - Added support for continuous integration with Github Actions.
 - `storm-pars`: Exploit monotonicity for computing extremal values and parameter space partitioning.

resources/examples/testfiles/mdp/die_selection.nm

@@ -1,4 +1,4 @@
-// Knuth's model of a fair die using only fair coins
+// Variation to Knuth's model of a fair die using only fair coins
 mdp
 
 module die

src/storm/api/builder.h

@@ -83,6 +83,7 @@ namespace storm {
          * @tparam ValueType Type of the probabilities in the sparse model
          * @param model SymbolicModelDescription of the model
          * @param options Builder options
+         * @param actionMask An object to restrict which actions are expanded in the builder
          * @return A builder
          */
         template<typename ValueType>

src/storm/generator/NextStateGenerator.cpp

@@ -280,7 +280,7 @@ namespace storm {
         template<typename ValueType, typename StateType>
         storm::json<ValueType> NextStateGenerator<ValueType, StateType>::currentStateToJson(bool onlyObservable) const {
             storm::json<ValueType> result = unpackStateIntoJson<ValueType>(*state, variableInformation, onlyObservable);
-            extendStateInformation(result,onlyObservable);
+            extendStateInformation(result);
             return result;
         }
 
@@ -291,7 +291,7 @@ namespace storm {
 
 
         template<typename ValueType, typename StateType>
-        void NextStateGenerator<ValueType, StateType>::extendStateInformation(storm::json<ValueType>&, bool) const {
+        void NextStateGenerator<ValueType, StateType>::extendStateInformation(storm::json<ValueType>&) const {
             // Intentionally left empty.
         }
 

src/storm/generator/NextStateGenerator.h

@@ -39,14 +39,29 @@ namespace storm {
         template<typename ValueType, typename StateType = uint32_t>
         class NextStateGenerator;
 
+        /*!
+         * Action masks are arguments you can give to the state generator that limit which states are generated.
+         *
+         */
         template<typename ValueType, typename StateType = uint32_t>
         class ActionMask {
         public:
             virtual ~ActionMask() = default;
+            /**
+             * This method is called to check whether an action should be expanded.
+             * The current state is obtained from the generator.
+             *
+             * @param generator the generator that is to be masked
+             * @param actionIndex the actionIndex in Prism Programs; i.e. id of actions.
+             * @return true if the mask allows building the action/edge/command
+             */
             virtual bool query(storm::generator::NextStateGenerator<ValueType, StateType> const &generator, uint64_t actionIndex) = 0;
         };
 
-
+        /*!
+         * A particular instance of the action mask that uses a callback function
+         * to evaluate whether an action should be expanded.
+         */
         template<typename ValueType, typename StateType = uint32_t>
         class StateValuationFunctionMask : public ActionMask<ValueType,StateType> {
         public:
@@ -139,7 +154,7 @@ namespace storm {
             
             virtual storm::storage::BitVector evaluateObservationLabels(CompressedState const& state) const =0;
 
-            virtual void extendStateInformation(storm::json<ValueType>& stateInfo, bool onlyObservable = false) const;
+            virtual void extendStateInformation(storm::json<ValueType>& stateInfo) const;
 
             virtual storm::storage::sparse::StateValuationsBuilder initializeObservationValuationsBuilder() const;
 

src/storm/generator/PrismNextStateGenerator.cpp

@@ -793,7 +793,7 @@ namespace storm {
         }
 
         template<typename ValueType, typename StateType>
-        void PrismNextStateGenerator<ValueType, StateType>::extendStateInformation(storm::json<ValueType>& result, bool onlyObservable) const {
+        void PrismNextStateGenerator<ValueType, StateType>::extendStateInformation(storm::json<ValueType>& result) const {
 
             for (uint64_t i = 0; i < program.getNumberOfObservationLabels(); ++i) {
                 result[program.getObservationLabels()[i].getName()] = this->evaluator->asInt(program.getObservationLabels()[i].getStatePredicateExpression());

src/storm/generator/PrismNextStateGenerator.h

@@ -102,10 +102,10 @@ namespace storm {
              */
             void addLabeledChoices(std::vector<Choice<ValueType>>& choices, CompressedState const& state, StateToIdCallback stateToIdCallback, CommandFilter const& commandFilter = CommandFilter::All);
 
-            /*
+            /*!
              * Extend the Json struct with additional information about the state.
              */
-            virtual void extendStateInformation(storm::json<ValueType>& stateInfo, bool onlyObservable = false) const override;
+            virtual void extendStateInformation(storm::json<ValueType>& stateInfo) const override;
 
             /*!
              * Evaluate observation labels

src/storm/simulator/PrismProgramSimulator.h

@@ -39,6 +39,7 @@ namespace storm {
             /**
              *
              * @return A list of choices that encode the possibilities in the current state.
+             * @note successor states are encoded using state indices that will potentially be invalidated as soon as the internal state of the simulator changes
              */
             std::vector<generator::Choice<ValueType, uint32_t>> const& getChoices() const;
 

src/storm/storage/expressions/Expression.cpp

@@ -13,7 +13,7 @@
 #include "storm/exceptions/InvalidTypeException.h"
 #include "storm/exceptions/InvalidArgumentException.h"
 #include "storm/utility/macros.h"
-#include "storm/storage/expressions/SimplificationVisitor.h"
+#include "storm/storage/expressions/RestrictSyntaxVisitor.h"
 
 namespace storm {
     namespace expressions {
@@ -55,7 +55,7 @@ namespace storm {
 		}
 
 		Expression Expression::substituteNonStandardPredicates() const {
-            return SimplificationVisitor().substitute(*this);
+            return RestrictSyntaxVisitor().substitute(*this);
         }
 
         bool Expression::evaluateAsBool(Valuation const* valuation) const {

src/storm/storage/expressions/ExpressionStringFormat.h

@@ -1,9 +0,0 @@
-#pragma once
-
-namespace storm {
-    namespace expressions {
-        struct ExpressionStringFormat {
-            bool alternativeIff = false;
-        };
-    }
-}

src/storm/storage/expressions/PredicateExpression.cpp

@@ -26,16 +26,16 @@ namespace storm {
 
         bool PredicateExpression::evaluateAsBool(Valuation const *valuation) const {
             STORM_LOG_THROW(this->hasBooleanType(), storm::exceptions::InvalidTypeException, "Unable to evaluate expression as boolean.");
-            storm::storage::BitVector results(operands.size());
-            uint64_t i = 0;
+            uint64_t nrTrue = 0;
             for(auto const& operand : operands) {
-                results.set(i, operand->evaluateAsBool(valuation));
-                ++i;
+                if (operand->evaluateAsBool(valuation)) {
+                    nrTrue++;
+                }
             }
             switch(predicate) {
-                case PredicateType::ExactlyOneOf: return results.getNumberOfSetBits() == 1;
-                case PredicateType::AtMostOneOf: return results.getNumberOfSetBits() <= 1;
-                case PredicateType::AtLeastOneOf: return results.getNumberOfSetBits() >= 1;
+                case PredicateType::ExactlyOneOf: return nrTrue == 1;
+                case PredicateType::AtMostOneOf: return nrTrue <= 1;
+                case PredicateType::AtLeastOneOf: return nrTrue >= 1;
             }
             STORM_LOG_ASSERT(false, "Unknown predicate type");
         }
@@ -45,7 +45,14 @@ namespace storm {
             for (auto const& operand : operands) {
                 simplifiedOperands.push_back(operand->simplify());
             }
-            return std::shared_ptr<BaseExpression>(new PredicateExpression(this->getManager(), this->getType(), simplifiedOperands, predicate));
+            // Return new expression if something changed.
+            for (uint64_t i = 0; i < operands.size(); ++i) {
+                if (operands[i] != simplifiedOperands[i]) {
+                    return std::shared_ptr<BaseExpression>(new PredicateExpression(this->getManager(), this->getType(), simplifiedOperands, predicate));
+                }
+            }
+            // All operands remained the same.
+            return this->shared_from_this();
         }
 
         boost::any PredicateExpression::accept(ExpressionVisitor &visitor, boost::any const &data) const {

src/storm/storage/expressions/SimplificationVisitor.cpp → src/storm/storage/expressions/RestrictSyntaxVisitor.cpp

@@ -2,23 +2,23 @@
 #include <unordered_map>
 #include <string>
 
-#include "storm/storage/expressions/SimplificationVisitor.h"
+#include "storm/storage/expressions/RestrictSyntaxVisitor.h"
 #include "storm/storage/expressions/Expressions.h"
 #include "storm/storage/expressions/PredicateExpression.h"
 #include "storm/storage/expressions/ExpressionManager.h"
 
 namespace storm {
     namespace expressions {
-        SimplificationVisitor::SimplificationVisitor() {
+        RestrictSyntaxVisitor::RestrictSyntaxVisitor() {
             // Intentionally left empty.
         }
 
-        Expression SimplificationVisitor::substitute(Expression const &expression) {
+        Expression RestrictSyntaxVisitor::substitute(Expression const &expression) {
             return Expression(boost::any_cast<std::shared_ptr<BaseExpression const>>(
                     expression.getBaseExpression().accept(*this, boost::none)));
         }
 
-        boost::any SimplificationVisitor::visit(IfThenElseExpression const &expression, boost::any const &data) {
+        boost::any RestrictSyntaxVisitor::visit(IfThenElseExpression const &expression, boost::any const &data) {
             std::shared_ptr<BaseExpression const> conditionExpression = boost::any_cast<std::shared_ptr<BaseExpression const>>(
                     expression.getCondition()->accept(*this, data));
             std::shared_ptr<BaseExpression const> thenExpression = boost::any_cast<std::shared_ptr<BaseExpression const>>(
@@ -39,7 +39,7 @@ namespace storm {
         }
 
         boost::any
-        SimplificationVisitor::visit(BinaryBooleanFunctionExpression const &expression, boost::any const &data) {
+        RestrictSyntaxVisitor::visit(BinaryBooleanFunctionExpression const &expression, boost::any const &data) {
             std::shared_ptr<BaseExpression const> firstExpression = boost::any_cast<std::shared_ptr<BaseExpression const>>(
                     expression.getFirstOperand()->accept(*this, data));
             std::shared_ptr<BaseExpression const> secondExpression = boost::any_cast<std::shared_ptr<BaseExpression const>>(
@@ -58,7 +58,7 @@ namespace storm {
         }
 
         boost::any
-        SimplificationVisitor::visit(BinaryNumericalFunctionExpression const &expression, boost::any const &data) {
+        RestrictSyntaxVisitor::visit(BinaryNumericalFunctionExpression const &expression, boost::any const &data) {
             std::shared_ptr<BaseExpression const> firstExpression = boost::any_cast<std::shared_ptr<BaseExpression const>>(
                     expression.getFirstOperand()->accept(*this, data));
             std::shared_ptr<BaseExpression const> secondExpression = boost::any_cast<std::shared_ptr<BaseExpression const>>(
@@ -76,7 +76,7 @@ namespace storm {
             }
         }
 
-        boost::any SimplificationVisitor::visit(BinaryRelationExpression const &expression, boost::any const &data) {
+        boost::any RestrictSyntaxVisitor::visit(BinaryRelationExpression const &expression, boost::any const &data) {
             std::shared_ptr<BaseExpression const> firstExpression = boost::any_cast<std::shared_ptr<BaseExpression const>>(
                     expression.getFirstOperand()->accept(*this, data));
             std::shared_ptr<BaseExpression const> secondExpression = boost::any_cast<std::shared_ptr<BaseExpression const>>(
@@ -93,14 +93,14 @@ namespace storm {
             }
         }
 
-        boost::any SimplificationVisitor::visit(VariableExpression const &expression, boost::any const &) {
+        boost::any RestrictSyntaxVisitor::visit(VariableExpression const &expression, boost::any const &) {
 
             return expression.getSharedPointer();
 
         }
 
         boost::any
-        SimplificationVisitor::visit(UnaryBooleanFunctionExpression const &expression, boost::any const &data) {
+        RestrictSyntaxVisitor::visit(UnaryBooleanFunctionExpression const &expression, boost::any const &data) {
             std::shared_ptr<BaseExpression const> operandExpression = boost::any_cast<std::shared_ptr<BaseExpression const>>(
                     expression.getOperand()->accept(*this, data));
 
@@ -115,7 +115,7 @@ namespace storm {
         }
 
         boost::any
-        SimplificationVisitor::visit(UnaryNumericalFunctionExpression const &expression, boost::any const &data) {
+        RestrictSyntaxVisitor::visit(UnaryNumericalFunctionExpression const &expression, boost::any const &data) {
             std::shared_ptr<BaseExpression const> operandExpression = boost::any_cast<std::shared_ptr<BaseExpression const>>(
                     expression.getOperand()->accept(*this, data));
 
@@ -129,7 +129,7 @@ namespace storm {
             }
         }
 
-        boost::any SimplificationVisitor::visit(PredicateExpression const &expression, boost::any const &data) {
+        boost::any RestrictSyntaxVisitor::visit(PredicateExpression const &expression, boost::any const &data) {
             std::vector<Expression> newExpressions;
             for (uint64_t i = 0; i < expression.getArity(); ++i) {
                 newExpressions.emplace_back(boost::any_cast<std::shared_ptr<BaseExpression const>>(
@@ -155,15 +155,15 @@ namespace storm {
         }
 
 
-        boost::any SimplificationVisitor::visit(BooleanLiteralExpression const &expression, boost::any const &) {
+        boost::any RestrictSyntaxVisitor::visit(BooleanLiteralExpression const &expression, boost::any const &) {
             return expression.getSharedPointer();
         }
 
-        boost::any SimplificationVisitor::visit(IntegerLiteralExpression const &expression, boost::any const &) {
+        boost::any RestrictSyntaxVisitor::visit(IntegerLiteralExpression const &expression, boost::any const &) {
             return expression.getSharedPointer();
         }
 
-        boost::any SimplificationVisitor::visit(RationalLiteralExpression const &expression, boost::any const &) {
+        boost::any RestrictSyntaxVisitor::visit(RationalLiteralExpression const &expression, boost::any const &) {
             return expression.getSharedPointer();
         }
 

src/storm/storage/expressions/SimplificationVisitor.h → src/storm/storage/expressions/RestrictSyntaxVisitor.h

@@ -6,7 +6,7 @@
 
 namespace storm {
     namespace expressions {
-        class SimplificationVisitor : public ExpressionVisitor {
+        class RestrictSyntaxVisitor : public ExpressionVisitor {
         public:
             /*!
              * Creates a new simplification visitor that replaces predicates by other (simpler?) predicates.
@@ -15,7 +15,7 @@ namespace storm {
              *  Currently, the visitor only replaces nonstandard predicates
              *
              */
-            SimplificationVisitor();
+            RestrictSyntaxVisitor();
 
             /*!
              * Simplifies based on the configuration.

src/storm/storage/expressions/ToDiceStringVisitor.cpp

@@ -1,5 +1,6 @@
 #include "storm/exceptions/NotSupportedException.h"
 #include "storm/storage/expressions/ToDiceStringVisitor.h"
+#include "storm/storage/expressions/Expressions.h"
 
 namespace storm {
     namespace expressions {

src/storm/storage/expressions/ToDiceStringVisitor.h

@@ -3,7 +3,6 @@
 #include <sstream>
 
 #include "storm/storage/expressions/Expression.h"
-#include "storm/storage/expressions/Expressions.h"
 #include "storm/storage/expressions/ExpressionVisitor.h"
 
 namespace storm {

src/storm/storage/prism/Program.h

@@ -6,8 +6,8 @@
 #include <vector>
 #include <set>
 #include <boost/optional.hpp>
-#include <storm/storage/BitVector.h>
 
+#include "storm/storage/BitVector.h"
 #include "storm/storage/prism/Constant.h"
 #include "storm/storage/prism/Formula.h"
 #include "storm/storage/prism/Label.h"
@@ -739,6 +739,10 @@ namespace storm {
              */
             std::pair<storm::jani::Model, std::vector<storm::jani::Property>> toJani(std::vector<storm::jani::Property> const& properties, bool allVariablesGlobal = true, std::string suffix = "") const;
 
+            /*!
+             * Compute the (labelled) commands in the program that may be synchronizing
+             * @return A bitvector representing the set of commands by global command index
+             */
             storm::storage::BitVector const& getPossiblySynchronizingCommands() const;
 
         private: