Corrected logging, formatting and cleaned up unuesed includes (accord…

…ing to the reviewer's comments)
klocs · Apr 13, 2019 · 4cf9bc8 · 4cf9bc8
1 parent 292b42d
commit 4cf9bc8
Show file tree

Hide file tree

Showing 5 changed files with 82 additions and 84 deletions.
diff --git a/src/slic3r.cpp b/src/slic3r.cpp
@@ -1,6 +1,7 @@
 #include "slic3r.hpp"
 #include "Geometry.hpp"
 #include "IO.hpp"
+#include "Log.hpp"
 #include "SLAPrint.hpp"
 #include "Print.hpp"
 #include "SimplePrint.hpp"
@@ -43,75 +44,67 @@ int CLI::run(int argc, char **argv) {
     this->config_def.merge(cli_misc_config_def);
     this->config_def.merge(print_config_def);
     this->config.def = &this->config_def;
-    std::cerr<<"Configs merged"<<std::endl;
+    Slic3r::Log::debug("CLI")<<"Configs merged.\n";
     // if any option is unsupported, print usage and abort immediately
     if (!this->config.read_cli(argc, argv, &this->input_files, &opt_order)) {
         this->print_help();
-        return 1;
+        exit(EXIT_FAILURE);
     }
     // parse actions and transform options
     for (auto const &opt_key : opt_order) {
         if (cli_actions_config_def.has(opt_key)) this->actions.push_back(opt_key);
         if (cli_transform_config_def.has(opt_key)) this->transforms.push_back(opt_key);
     }
-    try{
     // load config files supplied via --load
     for (auto const &file : config.getStrings("load")) {
-        if (!boost::filesystem::exists(file)) {
-            if (config.getBool("ignore_nonexistent_file", false)) {
-                continue;
-            } else {
-                boost::nowide::cerr << "No such file: " << file << std::endl;
-                exit(1);
+        try{
+            if (!boost::filesystem::exists(file)) {
+                if (config.getBool("ignore_nonexistent_file", false)) {
+                    continue;
+                } else {
+                    throw std::invalid_argument("No such file");
+                }
             }
-        }
-
-        DynamicPrintConfig c;
-        try {
+            DynamicPrintConfig c;
             c.load(file);
-        } catch (std::exception &e) {
-            boost::nowide::cerr << "Error while reading config file: " << e.what() << std::endl;
-            exit(1);
-        }
-        c.normalize();
-        this->print_config.apply(c);
-    }
-    }catch (...){
-	std::cerr<<"Exception in 'load' processing "<<std::endl;
+            c.normalize();
+            this->print_config.apply(c);
+        } catch (std::exception &e){
+	    Slic3r::Log::error("CLI") << "Error with the config file '" << file << "': " << e.what() <<std::endl;
+	    exit(EXIT_FAILURE);
+	}
     }
 
     // apply command line options to a more specific DynamicPrintConfig which provides normalize()
     // (command line options override --load files)
     this->print_config.apply(config, true);
     this->print_config.normalize();
-    std::cerr<<"Print_config normalized" << std::endl;
+    Slic3r::Log::debug("CLI") << "Print config normalized" << std::endl;
     // create a static (full) print config to be used in our logic
     this->full_print_config.apply(this->print_config);
-    std::cerr<<"Full_print_config created"<<std::endl;
+    Slic3r::Log::debug("CLI") << "Full print config created" << std::endl;
     // validate config
     try {
         this->full_print_config.validate();
-    } catch (InvalidOptionException &e) {
-        boost::nowide::cerr << e.what() << std::endl;
-        return 1;
+    } catch (std::exception &e) {
+	Slic3r::Log::error("CLI") << "Config validation error: "<< e.what() << std::endl;
+	exit(EXIT_FAILURE);
     }
-    std::cerr<<"Config validated"<<std::endl;
+    Slic3r::Log::debug("CLI") << "Config validated" << std::endl;
 
     // read input file(s) if any
     for (auto const &file : input_files) {
         Model model;
         try {
             model = Model::read_from_file(file);
         } catch (std::exception &e) {
-            boost::nowide::cerr << file << ": " << e.what() << std::endl;
-            exit(1);
+	    Slic3r::Log::error("CLI") << file << ": " << e.what() << std::endl;
+	    exit(EXIT_FAILURE);
         }
-
         if (model.objects.empty()) {
-            boost::nowide::cerr << "Error: file is empty: " << file << std::endl;
-            continue;
+	    Slic3r::Log::error("CLI") << "Error: file is empty: " << file << std::endl;
+	    continue;
         }
-
         this->models.push_back(model);
     }
 
@@ -197,8 +190,8 @@ int CLI::run(int argc, char **argv) {
         } else if (opt_key == "scale_to_fit") {
             const auto opt = config.opt<ConfigOptionPoint3>(opt_key);
             if (!opt->is_positive_volume()) {
-                boost::nowide::cerr << "--scale-to-fit requires a positive volume" << std::endl;
-                return 1;
+                Slic3r::Log::error("CLI") << "--scale-to-fit requires a positive volume" << std::endl;
+                exit(EXIT_FAILURE);
             }
             for (auto &model : this->models)
                 for (auto &o : model.objects)
@@ -264,8 +257,8 @@ int CLI::run(int argc, char **argv) {
             for (auto &model : this->models)
                 model.repair();
         } else {
-            boost::nowide::cerr << "error: option not implemented yet: " << opt_key << std::endl;
-            return 1;
+	    Slic3r::Log::error("CLI") << " option not implemented yet: " << opt_key << std::endl;
+	    exit(EXIT_FAILURE);
         }
     }
 
@@ -300,7 +293,7 @@ int CLI::run(int argc, char **argv) {
         } else if (opt_key == "export_3mf") {
             this->export_models(IO::TMF);
         } else if (opt_key == "export_sla") {
-            boost::nowide::cerr << "--export-sla is not implemented yet" << std::endl;
+            Slic3r::Log::error("CLI") << "--export-sla is not implemented yet" << std::endl;
         } else if (opt_key == "export_sla_svg") {
             for (const Model &model : this->models) {
                 SLAPrint print(&model); // initialize print with model
@@ -325,7 +318,7 @@ int CLI::run(int argc, char **argv) {
                 print.center = !this->config.has("center")
                     && !this->config.has("align_xy")
                     && !this->config.getBool("dont_arrange");
-                std::cerr<<"Arrange: "<<print.arrange<<", center: "<<print.center<<std::endl;
+                Slic3r::Log::error("CLI") << "Arrange: " << print.arrange<< ", center: " << print.center << std::endl;
                 print.set_model(model);
 
                 // start chronometer
@@ -337,10 +330,10 @@ int CLI::run(int argc, char **argv) {
                 try {
                     print.export_gcode(outfile);
                 } catch (std::runtime_error &e) {
-                    boost::nowide::cerr << e.what() << std::endl;
-                    return 1;
+                    Slic3r::Log::error("CLI") << e.what() << std::endl;
+                    exit(EXIT_FAILURE);
                 }
-                boost::nowide::cout << "G-code exported to " << outfile << std::endl;
+                Slic3r::Log::info("CLI") << "G-code exported to " << outfile << std::endl;
 
                 // output some statistics
                 double duration { std::chrono::duration_cast<second_>(clock_::now() - t0).count() };
@@ -353,8 +346,8 @@ int CLI::run(int argc, char **argv) {
                     << " (" << print.total_extruded_volume()/1000 << "cm3)" << std::endl;
             }
         } else {
-            boost::nowide::cerr << "error: option not supported yet: " << opt_key << std::endl;
-            return 1;
+            Slic3r::Log::error("CLI") <<  "error: option not supported yet: " << opt_key << std::endl;
+            exit(EXIT_FAILURE);
         }
     }
 
@@ -366,7 +359,7 @@ int CLI::run(int argc, char **argv) {
         GUI::App::SetInstance(gui);
         wxEntry(argc, argv);
 #else
-        std::cout << "GUI support has not been built." << "\n";
+        Slic3r::Log::error("CLI") << "GUI support has not been built." << "\n";
 #endif   
     }
 

diff --git a/xs/src/libslic3r/ConfigBase.cpp b/xs/src/libslic3r/ConfigBase.cpp
@@ -372,33 +372,31 @@ void
 ConfigBase::apply_only(const ConfigBase &other, const t_config_option_keys &opt_keys, bool ignore_nonexistent, bool default_nonexistent) {
     // loop through options and apply them
     for (const t_config_option_key &opt_key : opt_keys) {
-        try{ 
-        ConfigOption* my_opt = this->option(opt_key, true);
-        if (opt_key.size() == 0) continue;
-        if (my_opt == NULL) {
-            if (ignore_nonexistent == false) throw UnknownOptionException(opt_key);
-            continue;
-        }
-        if (default_nonexistent && !other.has(opt_key)) {
-            auto* def_opt = this->def->get(opt_key).default_value->clone();
-            // not the most efficient way, but easier than casting pointers to subclasses
-            bool res = my_opt->deserialize( def_opt->serialize() );
-            if (!res) {
-                std::string error = "Unexpected failure when deserializing serialized value for " + opt_key;
-                CONFESS(error.c_str());
-            }
-            continue;
-        }
-
-        // not the most efficient way, but easier than casting pointers to subclasses
-        bool res = my_opt->deserialize( other.option(opt_key)->serialize() );
-        if (!res) {
-            std::string error = "Unexpected failure when deserializing serialized value for " + opt_key;
-            CONFESS(error.c_str());
-        }
+        try{
+	    ConfigOption* my_opt = this->option(opt_key, true);
+	    if (opt_key.size() == 0) continue;
+	    if (my_opt == NULL) {
+	        if (ignore_nonexistent == false) throw UnknownOptionException(opt_key);
+	        continue;
+	    }
+	    if (default_nonexistent && !other.has(opt_key)) {
+	        auto* def_opt = this->def->get(opt_key).default_value->clone();
+	        // not the most efficient way, but easier than casting pointers to subclasses
+	        bool res = my_opt->deserialize( def_opt->serialize() );
+	        if (!res) {
+	            std::string error = "Unexpected failure when deserializing serialized value for " + opt_key;
+	            CONFESS(error.c_str());
+	        }
+	        continue;
+	    }
+	    // not the most efficient way, but easier than casting pointers to subclasses
+	    bool res = my_opt->deserialize( other.option(opt_key)->serialize() );
+	    if (!res) {
+		std::string error = "Unexpected failure when deserializing serialized value for " + opt_key;
+		CONFESS(error.c_str());
+	    }
         } catch ( UnknownOptionException & e ){
-
-    	    //    std::cerr<<e.what()<<std::endl;
+	    Slic3r::Log::warn("Config") << "Option " << opt_key << ": "  << e.what()<< std::endl;
         }
     }
 }
@@ -928,7 +926,7 @@ ConfigOptionPoint::deserialize(std::string str, bool append) {
         this->value.x = boost::lexical_cast<coordf_t>(tokens[0]);
         this->value.y = boost::lexical_cast<coordf_t>(tokens[1]);
     } catch (boost::bad_lexical_cast &e){
-        std::cout << "Config option deserialisation error of ["<<str<<"]  : " << e.what()<<" (expected 2D point)" << std::endl;
+        Slic3r::Log::error("Config") << "Deserialisation error of [" << str << "]  : " << e.what()<<" (expected 2D point)" << std::endl;
         return false;
     }
     return true;
@@ -943,7 +941,7 @@ ConfigOptionPoint3::deserialize(std::string str, bool append) {
         this->value.y = boost::lexical_cast<coordf_t>(tokens[1]);
         this->value.z = boost::lexical_cast<coordf_t>(tokens[2]);
     } catch (boost::bad_lexical_cast &e){
-        std::cout << "Config option deserialisation error of ["<<str<<"]  : " << e.what()<<" (expected 3D point)" << std::endl;
+        Slic3r::Log::error("Config") << "Deserialisation error of ["<<str<<"]  : " << e.what()<<" (expected 3D point)" << std::endl;
         return false;
     }
     return true;
@@ -965,10 +963,9 @@ ConfigOptionPoints::deserialize(std::string str, bool append) {
 			this->values.push_back(point);
 		}
 	} catch (boost::bad_lexical_cast &e) {
-		printf("%s\n", e.what());
+		Slic3r::Log::error("Config") << "Deserialisation error of ["  << str << "] " << e.what() << " (expected list of points) ";
 		return false;
 	}
-
 	return true;
 }
 

diff --git a/xs/src/libslic3r/ConfigBase.hpp b/xs/src/libslic3r/ConfigBase.hpp
@@ -1,9 +1,6 @@
 #ifndef slic3r_ConfigBase_hpp_
 #define slic3r_ConfigBase_hpp_
 
-#include <stdio.h>
-#include <stdlib.h>
-
 #include <map>
 #include <climits>
 #include <cstdio>
@@ -43,16 +40,22 @@ class ConfigOptionException : public std::exception {
         s += this->opt_key;
         return s.c_str();
     }
-
 };
+
 class UnknownOptionException : public ConfigOptionException {
     using ConfigOptionException::ConfigOptionException;
 
 };
+
 class InvalidOptionException : public ConfigOptionException {
     using ConfigOptionException::ConfigOptionException;
 
-
+    public:
+	virtual const char* what() const noexcept {
+	    std::string s("Invalid value for option: ");
+	    s += this->opt_key;
+	    return s.c_str();
+	}
 };
 
 /// Specialization of std::exception to indicate that an unsupported accessor was called on a config option.

diff --git a/xs/src/libslic3r/PrintGCode.cpp b/xs/src/libslic3r/PrintGCode.cpp
@@ -1,6 +1,6 @@
 #include "PrintGCode.hpp"
 #include "PrintConfig.hpp"
-
+#include "Log.hpp"
 #include <ctime>
 #include <iostream>
 
@@ -406,7 +406,10 @@ PrintGCode::process_layer(size_t idx, const Layer* layer, const Points& copies)
         for (auto region_id = 0U; region_id < _print.regions.size(); ++region_id) {
             const PrintRegion* region = _print.get_region(region_id);
             if( region_id >= layer->region_count() ){
-		std::cerr<<"Layer #"<<layer->id()<<" doesn't have region "<<region_id<<". The layer has only "<<layer->region_count()<<" regions."<<std::endl;
+		Slic3r::Log::error("Layer processing") << "Layer #" << layer->id() 
+		    << " doesn't have region " << region_id << ". "
+		    << " The layer has " << layer->region_count() << " regions."
+		    << std::endl;
 		break;
 	    }
             const LayerRegion* layerm = layer->get_region(region_id);

diff --git a/xs/src/libslic3r/SupportMaterial.cpp b/xs/src/libslic3r/SupportMaterial.cpp
@@ -1,4 +1,5 @@
 #include "SupportMaterial.hpp"
+#include "Log.hpp"
 
 namespace Slic3r
 {
@@ -71,7 +72,8 @@ SupportMaterial::generate(PrintObject *object)
 
     //bugfix: do not try to generate overhang if there is no contact area 
     if( contact.empty() ){
-	std::cerr<<"Empty contact_areas of SupportMaterial for object ["<<object->model_object()->name<<"]"<<std::endl;
+	Slic3r::Log::error("Support material") << "Empty contact_areas of SupportMaterial for object "
+	    <<"[" << object->model_object()->name << "]" << std::endl;
     	return;
     }