Related to #399, reverted assignment evaluation order to be the same as in 2013.01 as the new implementation broke existing scripts. Added some experimental commented out code, which can be used as reference in the future

8 files changed, 72 insertions, 10 deletions

diff --git a/src/localscope.cc b/src/localscope.cc
index eecff91..dd40563 100644
--- a/src/localscope.cc
+++ b/src/localscope.cc
@@ -55,9 +55,11 @@ std::vector<AbstractNode*> LocalScope::instantiateChildren(const Context *evalct
 		// c->functions_p = &this->functions;
 		// c->modules_p = &this->modules;
 
-		BOOST_FOREACH (const Assignment &ass, this->assignments) {
-			c->set_variable(ass.first, ass.second->evaluate(c));
-		}
+		// Uncommenting the following would allow assignments in local scopes,
+		// but would cause duplicate evaluation of module scopes
+		// BOOST_FOREACH (const Assignment &ass, this->assignments) {
+		// 	c->set_variable(ass.first, ass.second->evaluate(c));
+		// }
 	}
 
 	std::vector<AbstractNode*> childnodes;
diff --git a/src/modcontext.cc b/src/modcontext.cc
index 8799af9..5b48009 100644
--- a/src/modcontext.cc
+++ b/src/modcontext.cc
@@ -17,6 +17,51 @@ ModuleContext::~ModuleContext()
 {
 }
 
+// Experimental code. See issue #399
+#if 0
+void ModuleContext::evaluateAssignments(const AssignmentList &assignments)
+{
+	// First, assign all simple variables
+	std::list<std::string> undefined_vars;
+ 	BOOST_FOREACH(const Assignment &ass, assignments) {
+		Value tmpval = ass.second->evaluate(this);
+		if (tmpval.isUndefined()) undefined_vars.push_back(ass.first);
+ 		else this->set_variable(ass.first, tmpval);
+ 	}
+
+	// Variables which couldn't be evaluated in the first pass is attempted again,
+  // to allow for initialization out of order
+
+	boost::unordered_map<std::string, Expression *> tmpass;
+	BOOST_FOREACH (const Assignment &ass, assignments) {
+		tmpass[ass.first] = ass.second;
+	}
+		
+	bool changed = true;
+	while (changed) {
+		changed = false;
+		std::list<std::string>::iterator iter = undefined_vars.begin();
+		while (iter != undefined_vars.end()) {
+			std::list<std::string>::iterator curr = iter++;
+			boost::unordered_map<std::string, Expression *>::iterator found = tmpass.find(*curr);
+			if (found != tmpass.end()) {
+				const Expression *expr = found->second;
+				Value tmpval = expr->evaluate(this);
+				// FIXME: it's not enough to check for undefined;
+				// we need to check for any undefined variable in the subexpression
+				// For now, ignore this and revisit the validity and order of variable
+				// assignments later
+				if (!tmpval.isUndefined()) {
+					changed = true;
+					this->set_variable(*curr, tmpval);
+					undefined_vars.erase(curr);
+				}
+			}
+		}
+	}
+}
+#endif
+
 void ModuleContext::initializeModule(const class Module &module)
 {
 	this->setVariables(module.definition_arguments, evalctx);
@@ -26,6 +71,8 @@ void ModuleContext::initializeModule(const class Module &module)
 	BOOST_FOREACH(const Assignment &ass, module.scope.assignments) {
 		this->set_variable(ass.first, ass.second->evaluate(this));
 	}
+// Experimental code. See issue #399
+//	evaluateAssignments(module.scope.assignments);
 }
 
 /*!
diff --git a/src/modcontext.h b/src/modcontext.h
index 0b3e48c..3a05a0c 100644
--- a/src/modcontext.h
+++ b/src/modcontext.h
@@ -36,6 +36,9 @@ public:
 #ifdef DEBUG
 	virtual void dump(const class AbstractModule *mod, const ModuleInstantiation *inst);
 #endif
+private:
+// Experimental code. See issue #399
+//	void evaluateAssignments(const AssignmentList &assignments);
 };
 
 class FileContext : public ModuleContext
diff --git a/src/parser.y b/src/parser.y
index 0977efa..5b3d019 100644
--- a/src/parser.y
+++ b/src/parser.y
@@ -136,15 +136,17 @@ statement inner_input ;
 
 assignment:
 TOK_ID '=' expr ';' {
+  bool found = false;
   for (AssignmentList::iterator iter = scope_stack.top()->assignments.begin(); 
        iter != scope_stack.top()->assignments.end(); 
        iter++) {
     if (iter->first == $1) {
-      scope_stack.top()->assignments.erase(iter);
+      iter->second = $3;
+      found = true;
       break;
     }
   }
-  scope_stack.top()->assignments.push_back(Assignment($1, $3));
+  if (!found) scope_stack.top()->assignments.push_back(Assignment($1, $3));
 } ;
 
 statement:
diff --git a/testdata/scad/misc/value-reassignment-tests.scad b/testdata/scad/misc/value-reassignment-tests.scad
index 4370c11..26afa03 100644
--- a/testdata/scad/misc/value-reassignment-tests.scad
+++ b/testdata/scad/misc/value-reassignment-tests.scad
@@ -4,6 +4,6 @@
 
 myval = 2;
 i = 2;
-myval = i * 2;
-echo(myval, i); // Should output 4, 2
+myval = i * 2; // This is not (yet) allowed as it will be evaluates in place of the first assignment
+echo(myval, i); // Should output undef, 2
 
diff --git a/testdata/scad/misc/value-reassignment-tests2.scad b/testdata/scad/misc/value-reassignment-tests2.scad
index 9d7cf50..29a2fb7 100644
--- a/testdata/scad/misc/value-reassignment-tests2.scad
+++ b/testdata/scad/misc/value-reassignment-tests2.scad
@@ -5,5 +5,12 @@
 myval = 2;
 i = myval;
 myval = 3;
-echo(myval, i); // Should output 3, 2
+echo(myval, i); // Should output 3, 3
+
+// NB! This feels wrong, but it's a simulation of what happens
+// when overriding a variable on the cmd-line: openscad -Dmyval=3 myfile.scad
+// Since the intention is to override a top-level variable, the evaluation of the
+// new expression must be done in the same place as the old.
+// This is currently solved by appending the text given to the -D parameter to the end
+// of the main file.
 
diff --git a/tests/regression/echotest/value-reassignment-tests-expected.txt b/tests/regression/echotest/value-reassignment-tests-expected.txt
index fc7b7b1..344f7ab 100644
--- a/tests/regression/echotest/value-reassignment-tests-expected.txt
+++ b/tests/regression/echotest/value-reassignment-tests-expected.txt
@@ -1 +1,2 @@
-ECHO: 4, 2
+WARNING: Ignoring unknown variable 'i'.
+ECHO: undef, 2
diff --git a/tests/regression/echotest/value-reassignment-tests2-expected.txt b/tests/regression/echotest/value-reassignment-tests2-expected.txt
index 99b37a2..efb1be7 100644
--- a/tests/regression/echotest/value-reassignment-tests2-expected.txt
+++ b/tests/regression/echotest/value-reassignment-tests2-expected.txt
@@ -1 +1 @@
-ECHO: 3, 2
+ECHO: 3, 3