Merge pull request #96 from clero/criterion_rework

Criterion API rework

Previous criterion API was error prone in many point:

 - exposition of internal components of a criterion
 - no check when setting a criterion
 - inclusive criterion were limited to 31 values
 - no check for inclusive criterion limit
 - Liskov principle was broken due to isInclusive method

This rework introduces a new criterion API which corrects those issues.

Author: dawagner

bindings/c/ParameterFramework.cpp

@@ -32,7 +32,6 @@
 #include <ParameterMgrPlatformConnector.h>
 
 #include <iostream>
-#include <limits>
 #include <string>
 #include <map>
 
@@ -41,12 +40,12 @@
 #include <cstdlib>
 
 using std::string;
-using core::criterion::CriterionInterface;
+using core::criterion::Criterion;
 
 /** Rename long pfw types to short ones in pfw namespace. */
 namespace pfw
 {
-    typedef std::map<string, CriterionInterface *> Criteria;
+    typedef std::map<string, Criterion*> Criteria;
     typedef CParameterMgrPlatformConnector Pfw;
 }
 
@@ -187,30 +186,19 @@ bool PfwHandler::createCriteria(const PfwCriterion criteriaArray[], size_t crite
                                   "\" already exist");
         }
 
-        // Create criterion
-        CriterionInterface *newCriterion = (criterion.inclusive ?
-                pfw->createInclusiveCriterion(criterion.name) :
-                pfw->createExclusiveCriterion(criterion.name));
-        assert(newCriterion != NULL);
-        // Add criterion values
+        // Create criterion values
+        core::criterion::Values values;
         for (size_t valueIndex = 0; criterion.values[valueIndex] != NULL; ++valueIndex) {
-            int value;
-            if (criterion.inclusive) {
-                // Check that (int)1 << valueIndex would not overflow (UB)
-                if(std::numeric_limits<int>::max() >> valueIndex == 0) {
-                    return status.failure("Too many values for criterion " +
-                                          string(criterion.name));
-                }
-                value = 1 << valueIndex;
-            } else {
-                value = valueIndex;
-            }
-            const char * valueName = criterion.values[valueIndex];
-            string error;
-            if(not newCriterion->addValuePair(value, valueName, error)) {
-                return status.failure("Could not add value " + string(valueName) +
-                                      " to criterion " + criterion.name + ": " + error);
-            }
+            values.emplace_back(criterion.values[valueIndex]);
+        }
+        // Create criterion
+        string error;
+        Criterion *newCriterion = (criterion.inclusive ?
+                pfw->createInclusiveCriterion(criterion.name, values, error) :
+                pfw->createExclusiveCriterion(criterion.name, values, error));
+        if (newCriterion == nullptr) {
+            return status.failure("Could not create criterion '" + string(criterion.name) +
+                                  "': " + error);
         }
         // Add new criterion to criteria list
         criteria[criterion.name] = newCriterion;
@@ -256,13 +244,13 @@ const char *pfwGetLastError(const PfwHandler *handle)
     return handle == NULL ? NULL : handle->lastStatus.msg().c_str();
 }
 
-static CriterionInterface *getCriterion(const pfw::Criteria &criteria, const string &name)
+static Criterion *getCriterion(const pfw::Criteria &criteria, const string &name)
 {
     pfw::Criteria::const_iterator it = criteria.find(name);
     return it == criteria.end() ? NULL : it->second;
 }
 
-bool pfwSetCriterion(PfwHandler *handle, const char name[], int value)
+bool pfwSetCriterion(PfwHandler *handle, const char name[], const char **values)
 {
     if (handle == NULL) { return Status::failure(); }
     Status &status = handle->lastStatus;
@@ -274,35 +262,64 @@ bool pfwSetCriterion(PfwHandler *handle, const char name[], int value)
         return status.failure("Can not set criterion \"" + string(name) +
                               "\" as the parameter framework is not started.");
     }
-    CriterionInterface *criterion = getCriterion(handle->criteria, name);
+    Criterion *criterion = getCriterion(handle->criteria, name);
     if (criterion == NULL) {
         return status.failure("Can not set criterion " + string(name) + " as does not exist");
     }
-    criterion->setCriterionState(value);
-    return status.success();
+    core::criterion::State state{};
+    for (size_t valueIndex = 0; values[valueIndex] != NULL; ++valueIndex) {
+        state.emplace(values[valueIndex]);
+    }
+    return status.forward(criterion->setState(state, status.msg()));
 }
-bool pfwGetCriterion(const PfwHandler *handle, const char name[], int *value)
+const char **pfwGetCriterion(const PfwHandler *handle, const char name[])
 {
-    if (handle == NULL) { return Status::failure(); }
+    if (handle == NULL) { return NULL; }
     Status &status = handle->lastStatus;
     if (name == NULL) {
-        return status.failure("char *name of the criterion is NULL, "
-                              "while getting a criterion.");
+        status.failure("char *name of the criterion is NULL, while getting a criterion.");
+        return NULL;
     }
     if (handle->pfw == NULL) {
-        return status.failure("Can not get criterion \"" + string(name) +
-                              "\" as the parameter framework is not started.");
-    }
-    if (value == NULL) {
-        return status.failure("Can not get criterion \"" + string(name) +
-                              "\" as the out value is NULL.");
+        status.failure("Can not get criterion \"" + string(name) +
+                       "\" as the parameter framework is not started.");
+        return NULL;
     }
-    CriterionInterface *criterion = getCriterion(handle->criteria, name);
+    Criterion *criterion = getCriterion(handle->criteria, name);
     if (criterion == NULL) {
-        return status.failure("Can not get criterion " + string(name) + " as it does not exist");
+        status.failure("Can not get criterion " + string(name) + " as it does not exist");
+        return NULL;
     }
-    *value = criterion->getCriterionState();
-    return status.success();
+    core::criterion::State state = criterion->getState();
+
+    // Allocating one slot for each criterion value plus one for the NULL end delimiter
+    const char **values = static_cast<const char**>(malloc((state.size() + 1) * sizeof(char*)));
+    if (values == nullptr) {
+        status.failure("Can not get criterion " + string(name) + " as there is no memory left");
+        return NULL;
+    }
+
+    size_t i = 0;
+    for (auto &critValue : state) {
+        values[i] = strdup(critValue.c_str());
+        if (values[i] == nullptr) {
+            status.failure("Can not get criterion " + string(name) +
+                           " as there is no memory left (errno: " + strerror(errno) + ")");
+            return NULL;
+        }
+        ++i;
+    }
+    values[i] = NULL;
+    return values;
+}
+
+void pfwCriterionValueFree(const char **value)
+{
+    const char **valueSav = value;
+    while(*value != NULL) {
+        std::free(*const_cast<char**>(value++));
+    }
+    std::free(valueSav);
 }
 
 bool pfwApplyConfigurations(const PfwHandler *handle)

bindings/c/ParameterFramework.h

@@ -156,25 +156,36 @@ const char *pfwGetLastError(const PfwHandler *handle) NONNULL;
 /** Set a criterion value given its name and value.
   * @param handle[in] @see PfwHandler
   * @param name[in] The name of the criterion that need to be changed.
-  * @param value If the criterion is exclusive, the index of the new value.
-  *              If the criterion is inclusive, a bit field where each bit
-  *              correspond to the value index.
+  * @param value If the criterion is exclusive, an array containing the new value.
+  *              If the criterion is inclusive, an array containing all new values.
   * For an inclusive criterion defined as such: { "Red", "Green", "Blue", NULL }
-  * to set the value Green and Blue, value has to be 1<<1 | 1<<2 = 0b110 = 6.
+  * to set the value Green and Blue, value has to be ["Green", "Blue", NULL].
   * For an exclusive criterion defined as such: { "Car", "Boat", "Plane", NULL }
-  * to set the value Plane, value has to be 2.
+  * to set the value Plane, value has to be ["Plane", NULL].
   *
   * Criterion change do not have impact on the parameters value
   * (no configuration applied) until the changes are committed using pfwApplyConfigurations.
   *
   * @return true on success and false on failure.
   */
-bool pfwSetCriterion(PfwHandler *handle, const char name[], int value) NONNULL USERESULT;
+bool pfwSetCriterion(PfwHandler *handle, const char name[], const char **values) NONNULL USERESULT;
 /** Get a criterion value given its name.
-  * Same usage as pfwSetCriterion except that value is an out param.
+  * @param handle[in] @see PfwHandler
+  * @param name[in] The name of the criterion that need to be changed.
+  * @return an NULL terminated array of char* which store criterion values if success
+  *         NULL otherwise
   * Get criterion will return the last value setted with pfwSetCriterion independantly of pfwCommitCritenio.
+  * The callee MUST free the returned value array using pfwCriterionValueFree after use.
+  */
+const char **pfwGetCriterion(const PfwHandler *handle, const char name[]) NONNULL USERESULT;
+
+/** Frees the memory space of criterion value array,
+  * which must have been returned by a previous call to pfwGetCriterion.
+  *
+  * @param value[in] pointer to the memory to free.
+  * @see man 3 free for usage.
   */
-bool pfwGetCriterion(const PfwHandler *handle, const char name[], int *value) NONNULL USERESULT;
+void pfwCriterionValueFree(const char **value);
 
 /** Commit criteria change and change parameters according to the configurations.
   * Criterion do not have impact on the parameters value when changed,

bindings/c/Test.cpp

@@ -182,18 +182,6 @@ TEST_CASE_METHOD(Test, "Parameter-framework c api use") {
             };
             REQUIRE_FAILURE(pfwStart(pfw, config, duplicatedCriteria, 2, &logger));
         }
-        WHEN("The pfw is started with duplicated criterion value state") {
-            const char * values[] = {"a", "a", NULL};
-            const PfwCriterion duplicatedCriteria[] = {{"name", true, values}};
-
-            WHEN("Using test logger") {
-                REQUIRE_FAILURE(pfwStart(pfw, config, duplicatedCriteria, 1, &logger));
-            }
-            WHEN("Using default logger") {
-                // Test coverage of default logger warning
-                REQUIRE_FAILURE(pfwStart(pfw, config, duplicatedCriteria, 1, NULL));
-            }
-        }
         WHEN("The pfw is started with NULL name criterion") {
             const PfwCriterion duplicatedCriteria[] = {{NULL, true, letterList}};
             REQUIRE_FAILURE(pfwStart(pfw, config, duplicatedCriteria, 1, &logger));
@@ -204,8 +192,8 @@ TEST_CASE_METHOD(Test, "Parameter-framework c api use") {
         }
         GIVEN("A criteria with lots of values")
         {
-            // Build a criterion with as many value as there is bits in int.
-            std::vector<char> names(sizeof(int) * CHAR_BIT + 1, 'a');
+            // Build a criterion with a lot of value
+            std::vector<char> names(300, 'a');
             names.back() = '\0';
             std::vector<const char *> values(names.size());
             for(size_t i = 0; i < values.size(); ++i) {
@@ -240,11 +228,7 @@ TEST_CASE_METHOD(Test, "Parameter-framework c api use") {
              */
             const PfwCriterion duplicatedCriteria[] = {{"name", true, &values[0]}};
 
-            WHEN("The pfw is started with a too long criterion state list") {
-                REQUIRE_FAILURE(pfwStart(pfw, config, duplicatedCriteria, 1, &logger));
-            }
-            WHEN("The pfw is started with max length criterion state list") {
-                values[values.size() - 2] = NULL; // Hide last value
+            WHEN("The pfw is started with a long criterion state list") {
                 REQUIRE_SUCCESS(pfwStart(pfw, config, duplicatedCriteria, 1, &logger));
             }
         }
@@ -267,11 +251,11 @@ TEST_CASE_METHOD(Test, "Parameter-framework c api use") {
         }
 
         WHEN("Get criterion of a stopped pfw") {
-            int value;
-            REQUIRE_FAILURE(pfwGetCriterion(pfw, criteria[0].name, &value));
+            REQUIRE_FAILURE(pfwGetCriterion(pfw, criteria[0].name) != NULL);
         }
         WHEN("Set criterion of a stopped pfw") {
-            REQUIRE_FAILURE(pfwSetCriterion(pfw, criteria[0].name, 1));
+            std::vector<const char*> value{"1", NULL};
+            REQUIRE_FAILURE(pfwSetCriterion(pfw, criteria[0].name, value.data()));
         }
         WHEN("Commit criteria of a stopped pfw") {
             REQUIRE_FAILURE(pfwApplyConfigurations(pfw));
@@ -284,55 +268,63 @@ TEST_CASE_METHOD(Test, "Parameter-framework c api use") {
         WHEN("The pfw is started correctly")
         {
             REQUIRE_SUCCESS(pfwStart(pfw, config, criteria, criterionNb, &logger));
-            int value;
+            int value = 0;
 
             WHEN("Get criterion without an handle") {
-                REQUIRE(not pfwGetCriterion(NULL, criteria[0].name, &value));
+                REQUIRE(pfwGetCriterion(NULL, criteria[0].name) == NULL);
             }
             WHEN("Get criterion without a name") {
-                REQUIRE_FAILURE(pfwGetCriterion(pfw, NULL, &value));
-            }
-            WHEN("Get criterion without an output value") {
-                REQUIRE_FAILURE(pfwGetCriterion(pfw, criteria[0].name, NULL));
+                REQUIRE_FAILURE(pfwGetCriterion(pfw, NULL) != NULL);
             }
             WHEN("Get not existing criterion") {
-                REQUIRE_FAILURE(pfwGetCriterion(pfw, "Do not exist", &value));
+                REQUIRE_FAILURE(pfwGetCriterion(pfw, "Do not exist") != NULL);
             }
-            THEN("All criterion should value 0") {
+            THEN("All criterion should have their default value") {
                 for(size_t i = 0; i < criterionNb; ++i) {
                     const char *criterionName = criteria[i].name;
                     CAPTURE(criterionName);
-                    REQUIRE_SUCCESS(pfwGetCriterion(pfw, criterionName, &value));
-                    REQUIRE(value == 0);
+                    const char **criterionGetValue = pfwGetCriterion(pfw, criterionName);
+                    REQUIRE_SUCCESS(criterionGetValue != NULL);
+                    REQUIRE(criterionGetValue != NULL);
+                    if (criteria[i].inclusive) {
+                        REQUIRE(criterionGetValue[0] == NULL);
+                    } else {
+                        REQUIRE(std::string(criterionGetValue[0]) == "1");
+                    }
+                    pfwCriterionValueFree(criterionGetValue);
                 }
             }
 
             WHEN("Set criterion without an handle") {
-                REQUIRE(not pfwSetCriterion(NULL, criteria[0].name, 1));
+                std::vector<const char*> setValue{"1", NULL};
+                REQUIRE(not pfwSetCriterion(NULL, criteria[0].name, setValue.data()));
             }
             WHEN("Set criterion without a name") {
-                REQUIRE_FAILURE(pfwSetCriterion(pfw, NULL, 2));
+                std::vector<const char*> setValue{"2", NULL};
+                REQUIRE_FAILURE(pfwSetCriterion(pfw, NULL, setValue.data()));
             }
             WHEN("Set not existing criterion") {
-                REQUIRE_FAILURE(pfwSetCriterion(pfw, "Do not exist", 3));
+                std::vector<const char*> setValue{"3", NULL};
+                REQUIRE_FAILURE(pfwSetCriterion(pfw, "Do not exist", setValue.data()));
             }
             WHEN("Set criterion value") {
                 for(size_t i = 0; i < criterionNb; ++i) {
+                    std::vector<const char*> setValue{(criteria[i].inclusive ? "b" : "2"), NULL};
                     const char *criterionName = criteria[i].name;
                     CAPTURE(criterionName);
-                    REQUIRE_SUCCESS(pfwSetCriterion(pfw, criterionName, 3));
-                }
-                THEN("Get criterion value should return what was set") {
-                    for(size_t i = 0; i < criterionNb; ++i) {
-                        const char *criterionName = criteria[i].name;
-                        CAPTURE(criterionName);
-                        REQUIRE_SUCCESS(pfwGetCriterion(pfw, criterionName, &value));
-                        REQUIRE(value == 3);
+                    REQUIRE_SUCCESS(pfwSetCriterion(pfw, criterionName, setValue.data()));
+                    THEN("Get criterion value should return what was set") {
+                        const char**criterionGetValue = pfwGetCriterion(pfw, criterionName);
+                        REQUIRE_SUCCESS(criterionGetValue != NULL);
+                        REQUIRE(std::string(criterionGetValue[0]) ==
+                                    (criteria[i].inclusive ? "b" : "2"));
+                        pfwCriterionValueFree(criterionGetValue);
                     }
                 }
                 WHEN("Set a new value to a criterion without committing first") {
+                    std::vector<const char*> setValue{"a", NULL};
                     const char *criterionName = criteria[0].name;
-                    REQUIRE_SUCCESS(pfwSetCriterion(pfw, criterionName, 0));
+                    REQUIRE_SUCCESS(pfwSetCriterion(pfw, criterionName, setValue.data()));
                     THEN("A warning message should have been displayed") {
                         INFO("Previous pfw log: \n" + logLines);
                         size_t logPos = logLines.find("Warning: Selection criterion "