Merge branch 'master' of github.com:openscad/openscad

12 files changed, 124 insertions, 65 deletions

diff --git a/examples/example018.scad b/examples/example018.scad
index 15ed745..8cd7fa5 100644
--- a/examples/example018.scad
+++ b/examples/example018.scad
@@ -2,7 +2,7 @@
 module step(len, mod)
 {
 	for (i = [0:$children-1])
-		translate([ len*(i - ($children-1)/2), 0, 0 ]) child((i+mod) % $children);
+		translate([ len*(i - ($children-1)/2), 0, 0 ]) children((i+mod) % $children);
 }
 
 for (i = [1:4])
diff --git a/src/ModuleCache.cc b/src/ModuleCache.cc
index 1b7a9e5..d79a026 100644
--- a/src/ModuleCache.cc
+++ b/src/ModuleCache.cc
@@ -22,51 +22,68 @@
 ModuleCache *ModuleCache::inst = NULL;
 
 /*!
-	Reevaluate the given file and recompile if necessary.
-	Returns NULL on any error (e.g. compile error or file not found)
+	Reevaluate the given file and all it's dependencies and recompile anything
+	needing reevaluation.
+	The given filename must be absolute.
 
-	If the given filename is relative, it means that the module hasn't been
-	previously located.
+	Sets the module reference to the new modile, or NULL on any error (e.g. compile
+	error or file not found)
+
+	Returns true if anything was compiled (module or dependencies) and false otherwise.
 */
-FileModule *ModuleCache::evaluate(const std::string &filename)
+bool ModuleCache::evaluate(const std::string &filename, FileModule *&module)
 {
-	FileModule *lib_mod = (this->entries.find(filename) != this->entries.end()) ?
-		&(*this->entries[filename].module) : NULL;
-
+	FileModule *lib_mod = NULL;
+	bool found = false;
+	if (this->entries.find(filename) != this->entries.end()) {
+		found = true;
+		lib_mod = this->entries[filename].module;
+	}
+  
 	// Don't try to recursively evaluate - if the file changes
 	// during evaluation, that would be really bad.
 	if (lib_mod && lib_mod->isHandlingDependencies()) return lib_mod;
 
-	bool shouldCompile = true;
-
 	// Create cache ID
 	struct stat st;
 	memset(&st, 0, sizeof(struct stat));
 	bool valid = (stat(filename.c_str(), &st) == 0);
 
-  // If file isn't there, just return and let the cache retain the old module
+	// If file isn't there, just return and let the cache retain the old module
 	if (!valid) return NULL;
-	
+
+	// If the file is present, we'll always cache some result
 	std::string cache_id = str(boost::format("%x.%x") % st.st_mtime % st.st_size);
 
-	// Lookup in cache
-	if (lib_mod) {
-		if (this->entries[filename].cache_id == cache_id) {
+	cache_entry &entry = this->entries[filename];
+	// Initialize entry, if new
+	if (!found) {
+		entry.module = NULL;
+		entry.cache_id = cache_id;
+	}
+  
+	bool shouldCompile = true;
+	if (found) {
+		// Files should only be recompiled if the cache ID changed
+		if (entry.cache_id == cache_id) {
 			shouldCompile = false;
-			if (lib_mod->includesChanged()) {
+			// Recompile if includes changed
+			if (lib_mod && lib_mod->includesChanged()) {
 				lib_mod = NULL;
 				shouldCompile = true;
 			}
 		}
 	}
-	else {
-		shouldCompile = valid;
-	}
+
+#ifdef DEBUG
+	// Causes too much debug output
+	//if (!shouldCompile) PRINTB("Using cached library: %s (%p)", filename % lib_mod);
+#endif
 
 	// If cache lookup failed (non-existing or old timestamp), compile module
 	if (shouldCompile) {
 #ifdef DEBUG
-		if (this->entries.find(filename) != this->entries.end()) {
+		if (found) {
 			PRINTB("Recompiling cached library: %s (%s)", filename % cache_id);
 		}
 		else {
@@ -84,37 +101,28 @@ FileModule *ModuleCache::evaluate(const std::string &filename)
 			textbuf << ifs.rdbuf();
 		}
 		textbuf << "\n" << commandline_commands;
-
+		
 		print_messages_push();
-
-		FileModule *oldmodule = NULL;
-		cache_entry e = { NULL, cache_id };
-		if (this->entries.find(filename) != this->entries.end()) {
-			oldmodule = this->entries[filename].module;
-		}
-		this->entries[filename] = e;
+		
+		FileModule *oldmodule = lib_mod;
 		
 		std::string pathname = boosty::stringy(fs::path(filename).parent_path());
 		lib_mod = dynamic_cast<FileModule*>(parse(textbuf.str().c_str(), pathname.c_str(), false));
 		PRINTB_NOCACHE("  compiled module: %p", lib_mod);
 		
-		if (lib_mod) {
-			// We defer deletion so we can ensure that the new module won't
-			// have the same address as the old
-			delete oldmodule;
-			this->entries[filename].module = lib_mod;
-		} else {
-			this->entries.erase(filename);
-		}
+		// We defer deletion so we can ensure that the new module won't
+		// have the same address as the old
+		if (oldmodule) delete oldmodule;
+		entry.module = lib_mod;
+		entry.cache_id = cache_id;
 		
 		print_messages_pop();
 	}
+	
+	module = lib_mod;
+	bool depschanged = lib_mod ? lib_mod->handleDependencies() : false;
 
-	if (lib_mod) {
-		lib_mod->handleDependencies();
-	}
-
-	return lib_mod;
+	return shouldCompile || depschanged;
 }
 
 void ModuleCache::clear()
@@ -124,7 +132,11 @@ void ModuleCache::clear()
 
 FileModule *ModuleCache::lookup(const std::string &filename)
 {
-	return (this->entries.find(filename) != this->entries.end()) ?
-		&(*this->entries[filename].module) : NULL;
+	return isCached(filename) ? this->entries[filename].module : NULL;
+}
+
+bool ModuleCache::isCached(const std::string &filename)
+{
+	return this->entries.find(filename) != this->entries.end();
 }
 
diff --git a/src/ModuleCache.h b/src/ModuleCache.h
index 7795ab7..0c15fbd 100644
--- a/src/ModuleCache.h
+++ b/src/ModuleCache.h
@@ -8,8 +8,9 @@ class ModuleCache
 {
 public:
 	static ModuleCache *instance() { if (!inst) inst = new ModuleCache; return inst; }
-	class FileModule *evaluate(const std::string &filename);
+	bool evaluate(const std::string &filename, class FileModule *&module);
 	class FileModule *lookup(const std::string &filename);
+	bool isCached(const std::string &filename);
 	size_t size() { return this->entries.size(); }
 	void clear();
 
diff --git a/src/control.cc b/src/control.cc
index d5f664e..1b58da1 100644
--- a/src/control.cc
+++ b/src/control.cc
@@ -161,6 +161,7 @@ AbstractNode *ControlModule::instantiate(const Context* /*ctx*/, const ModuleIns
 
 	if (type == CHILD)
 	{
+		PRINT("DEPRECATED: child() will be removed in future releases. Use children() instead.");
 		int n = 0;
 		if (evalctx->numArgs() > 0) {
 			double v;
diff --git a/src/module.cc b/src/module.cc
index 1f4059e..ccc475e 100644
--- a/src/module.cc
+++ b/src/module.cc
@@ -263,45 +263,58 @@ bool FileModule::handleDependencies()
 	if (this->is_handling_dependencies) return false;
 	this->is_handling_dependencies = true;
 
-	bool changed = false;
+	bool somethingchanged = false;
+	std::vector<std::pair<std::string,std::string> > updates;
 
 	// If a lib in usedlibs was previously missing, we need to relocate it
 	// by searching the applicable paths. We can identify a previously missing module
 	// as it will have a relative path.
-
-	// Iterating manually since we want to modify the container while iterating
-	FileModule::ModuleContainer::iterator iter = this->usedlibs.begin();
-	while (iter != this->usedlibs.end()) {
-		FileModule::ModuleContainer::iterator curr = iter++;
+	BOOST_FOREACH(std::string filename, this->usedlibs) {
 
 		bool wasmissing = false;
+		bool found = true;
+
 		// Get an absolute filename for the module
-		std::string filename = *curr;
 		if (!boosty::is_absolute(filename)) {
 			wasmissing = true;
 			fs::path fullpath = find_valid_path(this->path, filename);
-			if (!fullpath.empty()) filename = boosty::stringy(fullpath);
+			if (!fullpath.empty()) {
+				updates.push_back(std::make_pair(filename, boosty::stringy(fullpath)));
+				filename = boosty::stringy(fullpath);
+			}
+			else {
+				found = false;
+			}
 		}
 
-		FileModule *oldmodule = ModuleCache::instance()->lookup(filename);
-		FileModule *newmodule = ModuleCache::instance()->evaluate(filename);
-		// Detect appearance but not removal of files
-		if (newmodule && oldmodule != newmodule) {
-			changed = true;
+		if (found) {
+			bool wascached = ModuleCache::instance()->isCached(filename);
+			FileModule *oldmodule = ModuleCache::instance()->lookup(filename);
+			FileModule *newmodule;
+			bool changed = ModuleCache::instance()->evaluate(filename, newmodule);
+			// Detect appearance but not removal of files, and keep old module
+			// on compile errors (FIXME: Is this correct behavior?)
+			if (changed) {
 #ifdef DEBUG
-			PRINTB_NOCACHE("  %s: %p -> %p", filename % oldmodule % newmodule);
+				PRINTB_NOCACHE("  %s: %p -> %p", filename % oldmodule % newmodule);
 #endif
-		}
-		if (!newmodule) {
+			}
+			somethingchanged |= changed;
 			// Only print warning if we're not part of an automatic reload
-			if (!oldmodule && !wasmissing) {
+			if (!newmodule && !wascached && !wasmissing) {
 				PRINTB_NOCACHE("WARNING: Failed to compile library '%s'.", filename);
 			}
 		}
 	}
 
+	// Relative filenames which were located is reinserted as absolute filenames
+	typedef std::pair<std::string,std::string> stringpair;
+	BOOST_FOREACH(const stringpair &files, updates) {
+		this->usedlibs.erase(files.first);
+		this->usedlibs.insert(files.second);
+	}
 	this->is_handling_dependencies = false;
-	return changed;
+	return somethingchanged;
 }
 
 AbstractNode *FileModule::instantiate(const Context *ctx, const ModuleInstantiation *inst, const EvalContext *evalctx) const
diff --git a/testdata/modulecache-tests/README.txt b/testdata/modulecache-tests/README.txt
index 214acc5..3a123e7 100644
--- a/testdata/modulecache-tests/README.txt
+++ b/testdata/modulecache-tests/README.txt
@@ -124,3 +124,22 @@ o rm cascade*.scad
 o Verify that no rerendering was triggered (the 4 objects are still there)
 o ./cascade2.sh
 o Verify that everything reloads at once without flickering
+
+Test 15: Correct handling of compile errors in auto-reloaded modules
+--------
+o Turn on Automatic Reload and Compile
+o Open mainusingerror.scad
+o Verify that you get:
+  - Compiling library '.../error.scad'.
+  - Parser error in line 3: syntax error
+  - WARNING: Failed to compile library '.../error.scad'.
+  - Main file should keep compiling
+o Verify that the above doesn't repeat
+
+Test 16: Dependency tracking of underlying dependencies
+--------
+o Turn on Automatic Reload and Compile
+o Open mainsubsub.scad
+o Verify that you see a red cylinder
+o edit subdit/subsub.scad: Change color
+o Verify that color changes
diff --git a/testdata/modulecache-tests/mainsubsub.scad b/testdata/modulecache-tests/mainsubsub.scad
new file mode 100644
index 0000000..b87af98
--- /dev/null
+++ b/testdata/modulecache-tests/mainsubsub.scad
@@ -0,0 +1,2 @@
+use <subdir/sub.scad>
+sub();
diff --git a/testdata/modulecache-tests/mainusingerror.scad b/testdata/modulecache-tests/mainusingerror.scad
new file mode 100644
index 0000000..626f4aa
--- /dev/null
+++ b/testdata/modulecache-tests/mainusingerror.scad
@@ -0,0 +1,3 @@
+//mainusingerror.scad
+echo(version());
+use <error.scad>
diff --git a/testdata/modulecache-tests/moduleoverload.scad b/testdata/modulecache-tests/moduleoverload.scad
index 1928715..f8ebc6a 100644
--- a/testdata/modulecache-tests/moduleoverload.scad
+++ b/testdata/modulecache-tests/moduleoverload.scad
@@ -1,7 +1,7 @@
 use <mymodule-lib.scad>
 
 module mymodule() {
-  sphere();
+  sphere(5);
 }
 
 mymodule();
diff --git a/testdata/modulecache-tests/mymodule-lib.scad b/testdata/modulecache-tests/mymodule-lib.scad
index 9d68581..c6f30f7 100644
--- a/testdata/modulecache-tests/mymodule-lib.scad
+++ b/testdata/modulecache-tests/mymodule-lib.scad
@@ -1,3 +1,3 @@
 module mymodule() {
-  cylinder(center=true);
+  cylinder(r=5, center=true);
 }
diff --git a/testdata/modulecache-tests/subdir/sub.scad b/testdata/modulecache-tests/subdir/sub.scad
new file mode 100644
index 0000000..bf0fc44
--- /dev/null
+++ b/testdata/modulecache-tests/subdir/sub.scad
@@ -0,0 +1,5 @@
+use <subsub.scad>
+
+module sub() {
+    subsub();
+}
diff --git a/testdata/modulecache-tests/subdir/subsub.scad b/testdata/modulecache-tests/subdir/subsub.scad
new file mode 100644
index 0000000..c79d608
--- /dev/null
+++ b/testdata/modulecache-tests/subdir/subsub.scad
@@ -0,0 +1,3 @@
+module subsub() {
+    color("red") cylinder(r = 10, h = 5);
+}