Refactored ParseIdentifierExpression and GetObjectValue

6 files changed, 96 insertions, 86 deletions

diff --git a/src/GCodes/GCodeBuffer/StringParser.cpp b/src/GCodes/GCodeBuffer/StringParser.cpp
index c9462a47..8c5dcb3e 100644
--- a/src/GCodes/GCodeBuffer/StringParser.cpp
+++ b/src/GCodes/GCodeBuffer/StringParser.cpp
@@ -2231,16 +2231,19 @@ ExpressionValue StringParser::ParseNumber()
 	return retvalue;
 }
 
-// Parse an identifier
-void StringParser::ParseIdentifier(const StringRef& id, bool evaluate)
+// Parse an identifier expression
+ExpressionValue StringParser::ParseIdentifierExpression(StringBuffer& stringBuffer, bool evaluate)
 {
 	if (!isalpha(gb.buffer[readPointer]))
 	{
 		throw ConstructParseException("expected an identifier");
 	}
 
-	// TODO The following would be more efficient if instead of evaluating indices in [ ] and converting them back to strings,
-	// we passed a list of indices to reprap.GetObjectValue and just put markers in the variable name for where the indices were
+	String<MaxVariableNameLength> id;
+	ObjectExplorationContext context(ObjectModelReportFlags::none, ObjectModelEntryFlags::none);
+
+	// Loop parsing identifiers and index expressions
+	// When we come across an index expression, evaluate it, add it to the context, and place a marker in the identifier string.
 	char c;
 	while (isalpha((c = gb.buffer[readPointer])) || isdigit(c) || c == '_' || c == '.' || c == '[')
 	{
@@ -2258,39 +2261,33 @@ void StringParser::ParseIdentifier(const StringRef& id, bool evaluate)
 			{
 				throw ConstructParseException("expected integer expression");
 			}
-			id.catf("[%" PRIi32 "]", index.iVal);		//TODO overflow check (or do the earlier TODO, then we won't need this catf call)
-			++readPointer;
+			++readPointer;								// skip the ']'
+			context.ProvideIndex(index.iVal);
+			c = '^';									// add the marker
 		}
-		else if (id.cat(c))
+		if (id.cat(c))
 		{
 			throw ConstructParseException("variable name too long");;
 		}
 	}
-}
-
-// Parse an identifier expression
-ExpressionValue StringParser::ParseIdentifierExpression(StringBuffer& stringBuffer, bool evaluate)
-{
-	String<MaxVariableNameLength> varName;
-	ParseIdentifier(varName.GetRef(), evaluate);
 
 	// Check for the names of constants
-	if (varName.Equals("true"))
+	if (id.Equals("true"))
 	{
 		return ExpressionValue(true);
 	}
 
-	if (varName.Equals("false"))
+	if (id.Equals("false"))
 	{
 		return ExpressionValue(false);
 	}
 
-	if (varName.Equals("pi"))
+	if (id.Equals("pi"))
 	{
 		return ExpressionValue(Pi);
 	}
 
-	if (varName.Equals("iterations"))
+	if (id.Equals("iterations"))
 	{
 		const int32_t v = gb.MachineState().GetIterations();
 		if (v < 0)
@@ -2300,7 +2297,7 @@ ExpressionValue StringParser::ParseIdentifierExpression(StringBuffer& stringBuff
 		return ExpressionValue(v);
 	}
 
-	if (varName.Equals("result"))
+	if (id.Equals("result"))
 	{
 		int32_t rslt;
 		switch (gb.GetLastResult())
@@ -2321,7 +2318,7 @@ ExpressionValue StringParser::ParseIdentifierExpression(StringBuffer& stringBuff
 		return ExpressionValue(rslt);
 	}
 
-	if (varName.Equals("line"))
+	if (id.Equals("line"))
 	{
 		return ExpressionValue((int32_t)gb.MachineState().lineNumber);
 	}
@@ -2332,7 +2329,7 @@ ExpressionValue StringParser::ParseIdentifierExpression(StringBuffer& stringBuff
 	{
 		// It's a function call
 		ExpressionValue rslt = ParseExpression(stringBuffer, 0, evaluate);
-		if (varName.Equals("abs"))
+		if (id.Equals("abs"))
 		{
 			switch (rslt.type)
 			{
@@ -2352,37 +2349,37 @@ ExpressionValue StringParser::ParseIdentifierExpression(StringBuffer& stringBuff
 				rslt.Set(0);
 			}
 		}
-		else if (varName.Equals("sin"))
+		else if (id.Equals("sin"))
 		{
 			ConvertToFloat(rslt, evaluate);
 			rslt.fVal = sinf(rslt.fVal);
 		}
-		else if (varName.Equals("cos"))
+		else if (id.Equals("cos"))
 		{
 			ConvertToFloat(rslt, evaluate);
 			rslt.fVal = cosf(rslt.fVal);
 		}
-		else if (varName.Equals("tan"))
+		else if (id.Equals("tan"))
 		{
 			ConvertToFloat(rslt, evaluate);
 			rslt.fVal = tanf(rslt.fVal);
 		}
-		else if (varName.Equals("asin"))
+		else if (id.Equals("asin"))
 		{
 			ConvertToFloat(rslt, evaluate);
 			rslt.fVal = asinf(rslt.fVal);
 		}
-		else if (varName.Equals("acos"))
+		else if (id.Equals("acos"))
 		{
 			ConvertToFloat(rslt, evaluate);
 			rslt.fVal = acosf(rslt.fVal);
 		}
-		else if (varName.Equals("atan"))
+		else if (id.Equals("atan"))
 		{
 			ConvertToFloat(rslt, evaluate);
 			rslt.fVal = atanf(rslt.fVal);
 		}
-		else if (varName.Equals("atan2"))
+		else if (id.Equals("atan2"))
 		{
 			ConvertToFloat(rslt, evaluate);
 			SkipWhiteSpace();
@@ -2396,18 +2393,18 @@ ExpressionValue StringParser::ParseIdentifierExpression(StringBuffer& stringBuff
 			ConvertToFloat(nextOperand, evaluate);
 			rslt.fVal = atan2f(rslt.fVal, nextOperand.fVal);
 		}
-		else if (varName.Equals("sqrt"))
+		else if (id.Equals("sqrt"))
 		{
 			ConvertToFloat(rslt, evaluate);
 			rslt.fVal = sqrtf(rslt.fVal);
 		}
-		else if (varName.Equals("isnan"))
+		else if (id.Equals("isnan"))
 		{
 			ConvertToFloat(rslt, evaluate);
 			rslt.type = TYPE_OF(bool);
 			rslt.bVal = (isnan(rslt.fVal) != 0);
 		}
-		else if (varName.Equals("max"))
+		else if (id.Equals("max"))
 		{
 			for (;;)
 			{
@@ -2430,7 +2427,7 @@ ExpressionValue StringParser::ParseIdentifierExpression(StringBuffer& stringBuff
 				}
 			}
 		}
-		else if (varName.Equals("min"))
+		else if (id.Equals("min"))
 		{
 			for (;;)
 			{
@@ -2465,7 +2462,7 @@ ExpressionValue StringParser::ParseIdentifierExpression(StringBuffer& stringBuff
 		return rslt;
 	}
 
-	return reprap.GetObjectValue(*this, varName.c_str());
+	return reprap.GetObjectValue(*this, context, id.c_str());
 }
 
 GCodeException StringParser::ConstructParseException(const char *str) const
diff --git a/src/GCodes/GCodeBuffer/StringParser.h b/src/GCodes/GCodeBuffer/StringParser.h
index 4a0f88c4..bc5236f5 100644
--- a/src/GCodes/GCodeBuffer/StringParser.h
+++ b/src/GCodes/GCodeBuffer/StringParser.h
@@ -123,8 +123,6 @@ private:
 		pre(readPointer >= 0; isdigit(gb.buffer[readPointer]));
 	ExpressionValue ParseIdentifierExpression(StringBuffer& stringBuffer, bool evaluate) THROWS_GCODE_EXCEPTION
 		pre(readPointer >= 0; isalpha(gb.buffer[readPointer]));
-	void ParseIdentifier(const StringRef& id, bool evaluate) THROWS_GCODE_EXCEPTION
-		pre(readPointer >= 0);
 
 	void BalanceNumericTypes(ExpressionValue& val1, ExpressionValue& val2, bool evaluate) THROWS_GCODE_EXCEPTION;
 	void BalanceTypes(ExpressionValue& val1, ExpressionValue& val2, bool evaluate) THROWS_GCODE_EXCEPTION;
diff --git a/src/Movement/Move.cpp b/src/Movement/Move.cpp
index 2b98f536..cf0b2160 100644
--- a/src/Movement/Move.cpp
+++ b/src/Movement/Move.cpp
@@ -85,12 +85,12 @@ constexpr ObjectModelTableEntry Move::objectModelTable[] =
 	{ "Timeout",				OBJECT_MODEL_FUNC((int32_t)self->idleTimeout),											ObjectModelEntryFlags::none },
 
 	// 3. Move.Axis[] members
-	{ "Homed",					OBJECT_MODEL_FUNC_NOSELF(reprap.GetGCodes().IsAxisHomed(context.GetIndex(0))),			ObjectModelEntryFlags::none },
-	{ "Letter",					OBJECT_MODEL_FUNC_NOSELF(reprap.GetGCodes().GetAxisLetters()[context.GetIndex(0)]),		ObjectModelEntryFlags::none },
-	{ "Max",					OBJECT_MODEL_FUNC_NOSELF(reprap.GetPlatform().AxisMaximum(context.GetIndex(0))),		ObjectModelEntryFlags::none },
-	{ "Min",					OBJECT_MODEL_FUNC_NOSELF(reprap.GetPlatform().AxisMinimum(context.GetIndex(0))),		ObjectModelEntryFlags::none },
-	{ "UserPosition",			OBJECT_MODEL_FUNC_NOSELF(reprap.GetGCodes().GetUserCoordinate(context.GetIndex(0)), 3),	ObjectModelEntryFlags::none },
-	{ "Visible",				OBJECT_MODEL_FUNC_NOSELF(context.GetIndex(0) < reprap.GetGCodes().GetVisibleAxes()),	ObjectModelEntryFlags::none },
+	{ "Homed",					OBJECT_MODEL_FUNC_NOSELF(reprap.GetGCodes().IsAxisHomed(context.GetLastIndex())),		ObjectModelEntryFlags::none },
+	{ "Letter",					OBJECT_MODEL_FUNC_NOSELF(reprap.GetGCodes().GetAxisLetters()[context.GetLastIndex()]),	ObjectModelEntryFlags::none },
+	{ "Max",					OBJECT_MODEL_FUNC_NOSELF(reprap.GetPlatform().AxisMaximum(context.GetLastIndex())),		ObjectModelEntryFlags::none },
+	{ "Min",					OBJECT_MODEL_FUNC_NOSELF(reprap.GetPlatform().AxisMinimum(context.GetLastIndex())),		ObjectModelEntryFlags::none },
+	{ "UserPosition",			OBJECT_MODEL_FUNC_NOSELF(reprap.GetGCodes().GetUserCoordinate(context.GetLastIndex()), 3),	ObjectModelEntryFlags::none },
+	{ "Visible",				OBJECT_MODEL_FUNC_NOSELF(context.GetLastIndex() < (int32_t)reprap.GetGCodes().GetVisibleAxes()),	ObjectModelEntryFlags::none },
 };
 
 constexpr uint8_t Move::objectModelTableDescriptor[] = { 4, 9, 3, 2, 6 };
diff --git a/src/ObjectModel/ObjectModel.cpp b/src/ObjectModel/ObjectModel.cpp
index dbe1ffb4..824e6f4a 100644
--- a/src/ObjectModel/ObjectModel.cpp
+++ b/src/ObjectModel/ObjectModel.cpp
@@ -14,23 +14,42 @@
 #include <cstring>
 #include <General/SafeStrtod.h>
 
-void ObjectExplorationContext::AddIndex(unsigned int index)
+void ObjectExplorationContext::AddIndex(int32_t index)
 {
-	if (numIndices == MaxIndices)
+	if (numIndicesCounted == MaxIndices)
+	{
+		throw GCodeException(-1, -1, "Too many indices");
+	}
+	indices[numIndicesCounted] = index;
+	++numIndicesCounted;
+}
+
+void ObjectExplorationContext::AddIndex()
+{
+	if (numIndicesCounted == numIndicesProvided)
 	{
 		THROW_INTERNAL_ERROR;
 	}
-	indices[numIndices] = index;
-	++numIndices;
+	++numIndicesCounted;
 }
 
 void ObjectExplorationContext::RemoveIndex()
 {
-	if (numIndices == 0)
+	if (numIndicesCounted == 0)
 	{
 		THROW_INTERNAL_ERROR;
 	}
-	--numIndices;
+	--numIndicesCounted;
+}
+
+void ObjectExplorationContext::ProvideIndex(int32_t index)
+{
+	if (numIndicesProvided == MaxIndices)
+	{
+		throw GCodeException(-1, -1, "Too many indices");
+	}
+	indices[numIndicesProvided] = index;
+	++numIndicesProvided;
 }
 
 // Constructor
@@ -38,11 +57,20 @@ ObjectModel::ObjectModel() noexcept
 {
 }
 
-unsigned int ObjectExplorationContext::GetIndex(size_t n) const
+int32_t ObjectExplorationContext::GetIndex(size_t n) const
 {
-	if (n < numIndices)
+	if (n < numIndicesCounted)
 	{
-		return indices[numIndices - n - 1];
+		return indices[numIndicesCounted - n - 1];
+	}
+	THROW_INTERNAL_ERROR;
+}
+
+int32_t ObjectExplorationContext::GetLastIndex() const
+{
+	if (numIndicesCounted != 0)
+	{
+		return indices[numIndicesCounted - 1];
 	}
 	THROW_INTERNAL_ERROR;
 }
@@ -309,7 +337,7 @@ const ObjectModelTableEntry* ObjectModel::FindObjectModelTableEntry(uint8_t tabl
 
 /*static*/ const char* ObjectModel::GetNextElement(const char *id) noexcept
 {
-	while (*id != 0 && *id != '.' && *id != '[')
+	while (*id != 0 && *id != '.' && *id != '[' && *id != '^')
 	{
 		++id;
 	}
@@ -352,20 +380,13 @@ int ObjectModelTableEntry::IdCompare(const char *id) const noexcept
 		++id;
 		++n;
 	}
-	return (*n == 0 && (*id == 0 || *id == '.' || *id == '[')) ? 0
+	return (*n == 0 && (*id == 0 || *id == '.' || *id == '[' || *id == '^')) ? 0
 		: (*id > *n) ? 1
 			: -1;
 }
 
-// Get the value of an object. This version is called on the root of the tree.
-ExpressionValue ObjectModel::GetObjectValue(const StringParser& sp, const char *idString) const
-{
-	ObjectExplorationContext context(ObjectModelReportFlags::none, ObjectModelEntryFlags::none);
-	return GetObjectValue(sp, context, 0, idString);
-}
-
 // Get the value of an object
-ExpressionValue ObjectModel::GetObjectValue(const StringParser& sp, ObjectExplorationContext& context, uint8_t tableNumber, const char *idString) const
+ExpressionValue ObjectModel::GetObjectValue(const StringParser& sp, ObjectExplorationContext& context, const char *idString, uint8_t tableNumber) const
 {
 	const ObjectModelTableEntry *const e = FindObjectModelTableEntry(tableNumber, idString);
 	if (e == nullptr)
@@ -382,36 +403,28 @@ ExpressionValue ObjectModel::GetObjectValue(const StringParser& sp, ObjectExplor
 {
 	if (val.type == TYPE_OF(const ObjectModelArrayDescriptor*))
 	{
-		if (*idString != '[')
-		{
-			throw sp.ConstructParseException("can't select whole array");
-		}
-		const char *endptr;
-		const long index = SafeStrtol(idString + 1, &endptr);
-		if (endptr == idString + 1 || *endptr != ']')
+		if (*idString != '^')
 		{
-			throw sp.ConstructParseException("expected ']'");
+			throw sp.ConstructParseException("missing array index");
 		}
 
+		context.AddIndex();
 		ReadLocker lock(val.omadVal->lockPointer);
 
-		if (index < 0 || (size_t)index >= val.omadVal->GetNumElements(this, context))
+		if (context.GetLastIndex() < 0 || (size_t)context.GetLastIndex() >= val.omadVal->GetNumElements(this, context))
 		{
 			throw sp.ConstructParseException("array index out of bounds");
 		}
 
-		context.AddIndex(index);
 		const ExpressionValue arrayElement = val.omadVal->GetElement(this, context);
-		val = GetObjectValue(sp, context, arrayElement, endptr + 1);
-		context.RemoveIndex();
-		return val;
+		return GetObjectValue(sp, context, arrayElement, idString + 1);
 	}
 
 	if (val.type == TYPE_OF(const ObjectModel*))
 	{
 		if (*idString == '.')
 		{
-			return val.omVal->GetObjectValue(sp, context, val.param, idString + 1);
+			return val.omVal->GetObjectValue(sp, context, idString + 1, val.param);
 		}
 		throw sp.ConstructParseException((*idString == 0) ? "selected value has non-primitive type" : "syntax error in value selector string");
 	}
diff --git a/src/ObjectModel/ObjectModel.h b/src/ObjectModel/ObjectModel.h
index 467d7fdd..cb671e14 100644
--- a/src/ObjectModel/ObjectModel.h
+++ b/src/ObjectModel/ObjectModel.h
@@ -125,12 +125,16 @@ enum class ObjectModelEntryFlags : uint8_t
 class ObjectExplorationContext
 {
 public:
-	ObjectExplorationContext(ObjectModelReportFlags rf, ObjectModelEntryFlags ff) noexcept : numIndices(0), reportFlags(rf), filterFlags(ff) { }
+	ObjectExplorationContext(ObjectModelReportFlags rf, ObjectModelEntryFlags ff) noexcept
+		: numIndicesProvided(0), numIndicesCounted(0), reportFlags(rf), filterFlags(ff) { }
 
-	void AddIndex(unsigned int index) THROWS_GCODE_EXCEPTION;
+	void AddIndex(int32_t index) THROWS_GCODE_EXCEPTION;
+	void AddIndex() THROWS_GCODE_EXCEPTION;
 	void RemoveIndex() THROWS_GCODE_EXCEPTION;
-	unsigned int GetIndex(size_t n) const THROWS_GCODE_EXCEPTION;
-	size_t GetNumIndices() const noexcept { return numIndices; }
+	void ProvideIndex(int32_t index) THROWS_GCODE_EXCEPTION;
+	int32_t GetIndex(size_t n) const THROWS_GCODE_EXCEPTION;
+	int32_t GetLastIndex() const THROWS_GCODE_EXCEPTION;
+	size_t GetNumIndicesCounted() const noexcept { return numIndicesCounted; }
 	ObjectModelReportFlags GetReportFlags() const noexcept { return reportFlags; }
 	ObjectModelEntryFlags GetFilterFlags() const noexcept { return filterFlags; }
 	bool ShortFormReport() const noexcept { return ((uint16_t)reportFlags & (uint16_t)ObjectModelReportFlags::shortForm) != 0; }
@@ -139,8 +143,9 @@ public:
 private:
 	static constexpr size_t MaxIndices = 4;			// max depth of array nesting
 
-	size_t numIndices;								// the number of indices stored
-	unsigned int indices[MaxIndices];
+	size_t numIndicesProvided;						// the number of indices provided, when we are doing a value lookup
+	size_t numIndicesCounted;						// the number of indices passed in the search string
+	int32_t indices[MaxIndices];
 	ObjectModelReportFlags reportFlags;
 	ObjectModelEntryFlags filterFlags;
 };
@@ -163,8 +168,8 @@ public:
 	// Construct a JSON representation of those parts of the object model requested by the user. This version is called on the root of the tree.
 	void ReportAsJson(OutputBuffer *buf, const char *filter, ObjectModelReportFlags rf, ObjectModelEntryFlags ff) const THROWS_GCODE_EXCEPTION;
 
-	// Get the value of an object. This version is called on the root of the tree.
-	ExpressionValue GetObjectValue(const StringParser& sp, const char *idString) const THROWS_GCODE_EXCEPTION;
+	// Get the value of an object via the table
+	ExpressionValue GetObjectValue(const StringParser& sp, ObjectExplorationContext& context, const char *idString, uint8_t tableNumber = 0) const THROWS_GCODE_EXCEPTION;
 
 	// Function to report a value or object as JSON
 	void ReportItemAsJson(OutputBuffer *buf, ObjectExplorationContext& context, ExpressionValue val, const char *filter) const THROWS_GCODE_EXCEPTION;
@@ -179,9 +184,6 @@ protected:
 	// Report an entire array as JSON
 	void ReportArrayAsJson(OutputBuffer *buf, ObjectExplorationContext& context, const ObjectModelArrayDescriptor *omad, const char *filter) const THROWS_GCODE_EXCEPTION;
 
-	// Get the value of an object via the table
-	ExpressionValue GetObjectValue(const StringParser& sp, ObjectExplorationContext& context, uint8_t tableNumber, const char *idString) const THROWS_GCODE_EXCEPTION;
-
 	// Get the value of an object that we hold
 	ExpressionValue GetObjectValue(const StringParser& sp, ObjectExplorationContext& context, ExpressionValue val, const char *idString) const THROWS_GCODE_EXCEPTION;
 
diff --git a/src/Version.h b/src/Version.h
index acf0dfc0..6028f701 100644
--- a/src/Version.h
+++ b/src/Version.h
@@ -20,7 +20,7 @@
 #endif
 
 #ifndef DATE
-# define DATE "2020-01-12b1"
+# define DATE "2020-01-12b2"
 #endif
 
 #define AUTHORS "reprappro, dc42, chrishamm, t3p3, dnewman, printm3d"