From fd77363f184631ddf6012591612d8b50ac12a005 Mon Sep 17 00:00:00 2001
From: Grigoris Pavlakis <grigpavl@ece.auth.gr>
Date: Sun, 15 Mar 2020 00:21:38 +0200
Subject: [PATCH] Add warning for the Parameter setter's type safety
---
CMakeLists.txt | 1 -
inc/Parameters/Parameters.hpp | 5 +++
inc/Services/Parameter.hpp | 69 ++++++++++++++---------------------
src/Services/Parameter.cpp | 1 -
4 files changed, 32 insertions(+), 44 deletions(-)
create mode 100644 inc/Parameters/Parameters.hpp
delete mode 100644 src/Services/Parameter.cpp
diff --git a/CMakeLists.txt b/CMakeLists.txt
index d75c7315..d56b1c4e 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -33,7 +33,6 @@ add_library(common OBJECT
src/Helpers/TimeHelper.cpp
src/Services/EventReportService.cpp
src/Services/MemoryManagementService.cpp
- src/Services/Parameter.cpp
src/Services/ParameterService.cpp
src/Services/RequestVerificationService.cpp
src/Services/TestService.cpp
diff --git a/inc/Parameters/Parameters.hpp b/inc/Parameters/Parameters.hpp
new file mode 100644
index 00000000..d225fbfa
--- /dev/null
+++ b/inc/Parameters/Parameters.hpp
@@ -0,0 +1,5 @@
+#include "Services/Parameter.hpp"
+
+namespace SystemParameters {
+ // initialize all system parameters here
+}
\ No newline at end of file
diff --git a/inc/Services/Parameter.hpp b/inc/Services/Parameter.hpp
index 2587008d..20b8e8c5 100644
--- a/inc/Services/Parameter.hpp
+++ b/inc/Services/Parameter.hpp
@@ -9,76 +9,61 @@
* Fully compliant with the standard's requirements.
*
* @author Grigoris Pavlakis <grigpavl@ece.auth.gr>
+ * @author Athanasios Theocharis <athatheo@csd.auth.gr>
+ *
+ *
+ * The Parameter class implements a way of storing and updating system parameters
+ * of arbitrary size and type, while avoiding std::any and dynamic memory allocation.
+ *
+ * It is split in two parts: an abstract ParameterBase class which contains the setter,
+ * and a
*/
/**
* Useful type definitions
- *
+ * (DEPRECATED - MARK FOR REMOVAL)
* @typedef ParamId: the unique ID of a parameter, used for searching
*/
typedef uint16_t ParamId;
-/**
- * Parameter class - Breakdown of fields
+/*
+ * MILLION DOLLAR QUESTIONS:
+ * setCurrentValue is templated. Since Parameter (a template class) inherits ParameterBase
+ * (a class containing a template member), does a specialization of Parameter also specialize
+ * setCurrentValue? If not, we have a problem, since Parameter won't necessarily specialize
+ * setCurrentValue with the correct type => our setter is *not* typesafe.
*
- * @private ptr: Pointer of the function that will update the parameter
- * @private currentValue: The current (as in last good) value of the parameter
+ * Answer: NO! After a discussion on ##C++-general@Freenode, it turns out that this specialization
+ * does not happen! What this means is that, while a Parameter can be specialized as e.g. int, there
+ * is no way to prevent the setter from being called with a non-int (e.g. string or float) argument,
+ * resulting in an attempt to write data inside the Parameter that have a different type from what the
+ * template is carrying.
*
+ * Proof of concept:
+ * Parameter<int> ircTest = Parameter<int>(1337);
+ * ircTest.setCurrentValue("Hello this is a problem speaking");
+ * auto s = ircTest.getValueAsString();
*
+ * This snippet will overwrite the 4 bytes of the Parameter field with garbage and most importantly,
+ * it will do so silently.
*
- * Methods:
- * @public Parameter(uint32_t initialValue = 0, UpdatePtr newPtr = nullptr):
- * Create a new Parameter object with initialValue as its starting value and newPtr
- * as its update function pointer. Arguments initialValue and newPtr are optional, and have default values of
- * 0 and nullptr respectively.
*
- * @public setCurrentValue(): Changes the current value of the parameter
- * @public getCurrentValue(): Gets the current value of the parameter
*/
-// class DataField {
-// protected:
-// uint8_t sizeInBytes;
-// void* dataFieldAddress;
-// // possible race: setCurrentValue may be ran when dataFieldAddress is uninitialized
-// public:
-// template <typename DataType>
-// void setCurrentValue(DataType newValue) {
-// DataType* typedDataFieldAddress = reinterpret_cast<DataType*>(dataFieldAddress);
-// *typedDataFieldAddress = newValue;
-// }
-// };
-
-/**
- * Highly likely that valuePtr is redundant! Then, ParameterBase could only contain
- * virtual methods and act as a mere interface, moving all specific functionality to
- * the Parameter class and ensuring type safety.
- */
-
class ParameterBase {
protected:
uint8_t sizeInBytes;
- void* valuePtr = nullptr;
+ void* valuePtr;
public:
virtual String<ECSS_ST_20_MAX_STRING_LENGTH> getValueAsString() = 0;
template <typename ValueType>
void setCurrentValue(ValueType newVal) {
- if (valuePtr == nullptr) {
- std::cout << "THIS IS NULL!" << std::endl;
- }
*reinterpret_cast<ValueType*>(valuePtr) = newVal;
}
};
-/**
- * MILLION DOLLAR QUESTIONS:
- * setCurrentValue is templated. Since Parameter (a template class) inherits ParameterBase
- * (a class containing a template member), does a specialization of Parameter also specialize
- * setCurrentValue? If not, we have a problem, since Parameter won't necessarily specialize
- * setCurrentValue with the correct type => our setter is *not* typesafe.
- */
template <typename ValueType>
class Parameter : public ParameterBase {
diff --git a/src/Services/Parameter.cpp b/src/Services/Parameter.cpp
deleted file mode 100644
index 526f4d6e..00000000
--- a/src/Services/Parameter.cpp
+++ /dev/null
@@ -1 +0,0 @@
-#include "Services/Parameter.hpp"
--
GitLab