widelands-dev team mailing list archive

[Simon Eilting <s-eilting@xxxxxx>]

Simon Eilting has proposed merging lp:~s-eilting/widelands/bug-1203436 into lp:widelands.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1203436 in widelands: "Widelands relies on POSIX functions to be defined"
  https://bugs.launchpad.net/widelands/+bug/1203436

For more details, see:
https://code.launchpad.net/~s-eilting/widelands/bug-1203436/+merge/247773

Mostly, remove strdup, strcasecmp and related stuff (see bug 1203436, https://bugs.launchpad.net/widelands/+bug/1203436).
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~s-eilting/widelands/bug-1203436 into lp:widelands.

=== added file 'cmake/codecheck/rules/do_not_use_strcasecmp'
--- cmake/codecheck/rules/do_not_use_strcasecmp	1970-01-01 00:00:00 +0000
+++ cmake/codecheck/rules/do_not_use_strcasecmp	2015-01-27 21:00:25 +0000
@@ -0,0 +1,18 @@
+#!/usr/bin/python
+
+"""
+strcasecmp isn't available on win32
+"""
+
+error_msg = 'Do not use strcasecmp/strncasecmp. Use boost::iequals instead.'
+
+regexp = r'\bstrn?casecmp\s*\('
+
+allowed = [
+    'boost::iequals(a, b)',
+]
+
+forbidden = [
+    'strcasecmp(a, b)',
+    '!strncasecmp(a, b, l)',
+]

=== added file 'cmake/codecheck/rules/do_not_use_strdup'
--- cmake/codecheck/rules/do_not_use_strdup	1970-01-01 00:00:00 +0000
+++ cmake/codecheck/rules/do_not_use_strdup	2015-01-27 21:00:25 +0000
@@ -0,0 +1,21 @@
+#!/usr/bin/python
+
+"""
+strdup isn't available on win32
+"""
+
+error_msg = 'Do not use strdup/strndup. Use std::string instead.'
+
+regexp = r'\bstrn?dup\s*\('
+
+allowed = [
+    'y = std::string(x)',
+
+    'y = new char[strlen(x)+1]',
+    'std::copy(x, x+strlen(x)+1, y)',
+]
+
+forbidden = [
+    'y = strdup(x)',
+    'y = strndup(x)',
+]

=== modified file 'src/ai/ai_hints.cc'
--- src/ai/ai_hints.cc	2014-07-22 20:18:11 +0000
+++ src/ai/ai_hints.cc	2015-01-27 21:00:25 +0000
@@ -19,20 +19,10 @@
 
 #include "ai/ai_hints.h"
 
-#include <cstdlib>
-#include <cstring>
-
 #include "profile/profile.h"
 
-BuildingHints::~BuildingHints() {
-	free(renews_map_resource);
-	free(mines_);
-}
-
 BuildingHints::BuildingHints(Section* const section)
-   : renews_map_resource(nullptr),
-     mines_(nullptr),
-     log_producer_(section ? section->get_bool("logproducer") : false),
+   : log_producer_(section ? section->get_bool("logproducer") : false),
      stone_producer_(section ? section->get_bool("stoneproducer") : false),
      needs_water_(section ? section->get_bool("needs_water") : false),
      mines_water_(section ? section->get_bool("mines_water") : false),
@@ -43,11 +33,12 @@
      mountain_conqueror_(section ? section->get_bool("mountain_conqueror") : false),
      prohibited_till_(section ? section->get_int("prohibited_till", 0) : 0),
      forced_after_(section ? section->get_int("forced_after", 864000) : 0),  // 10 days default
-     mines_percent_(section ? section->get_int("mines_percent", 100) : 0) {
+     mines_percent_(section ? section->get_int("mines_percent", 100) : 0)
+{
 	if (section) {
-		if (char const* const s = section->get_string("renews_map_resource"))
-			renews_map_resource = strdup(s);
-		if (char const* const s = section->get_string("mines"))
-			mines_ = strdup(s);
+		if (section->has_val("renews_map_resource"))
+			renews_map_resource_ = section->get_string("renews_map_resource");
+		if (section->has_val("mines"))
+			mines_ = section->get_string("mines");
 	}
 }

=== modified file 'src/ai/ai_hints.h'
--- src/ai/ai_hints.h	2014-07-22 20:18:11 +0000
+++ src/ai/ai_hints.h	2015-01-27 21:00:25 +0000
@@ -21,6 +21,7 @@
 #define WL_AI_AI_HINTS_H
 
 #include <stdint.h>
+#include <string>
 
 #include "base/macros.h"
 
@@ -31,14 +32,21 @@
 /// special properties of a building.
 struct BuildingHints {
 	BuildingHints(Section*);
-	~BuildingHints();
-
-	char const* get_renews_map_resource() const {
-		return renews_map_resource;
+
+	bool renews_map_resource() const {
+		return !renews_map_resource_.empty();
+	}
+
+	std::string get_renews_map_resource() const {
+		return renews_map_resource_;
+	}
+
+	bool has_mines() const {
+		return !mines_.empty();
 	}
 
 	char const* get_mines() const {
-		return mines_;
+		return mines_.c_str();
 	}
 
 	bool is_logproducer() const {
@@ -87,8 +95,8 @@
 	}
 
 private:
-	char* renews_map_resource;
-	char* mines_;
+	std::string renews_map_resource_;
+	std::string mines_;
 	bool log_producer_;
 	bool stone_producer_;
 	bool needs_water_;

=== modified file 'src/ai/defaultai.cc'
--- src/ai/defaultai.cc	2015-01-24 11:30:20 +0000
+++ src/ai/defaultai.cc	2015-01-27 21:00:25 +0000
@@ -320,8 +320,8 @@
 		bo.mountain_conqueror_ = bh.is_mountain_conqueror();
 		bo.prohibited_till_ = bh.get_prohibited_till() * 1000;  // value in conf is in seconds
 		bo.forced_after_ = bh.get_forced_after() * 1000;        // value in conf is in seconds
-		if (char const* const s = bh.get_renews_map_resource()) {
-			bo.production_hint_ = tribe_->safe_ware_index(s);
+		if (bh.renews_map_resource()) {
+			bo.production_hint_ = tribe_->safe_ware_index(bh.get_renews_map_resource());
 		}
 
 		// I just presume cut wood is named "log" in the game
@@ -345,8 +345,8 @@
 
 			if (bo.type == BuildingObserver::MINE) {
 				// get the resource needed by the mine
-				if (char const* const s = bh.get_mines()) {
-					bo.mines_ = world.get_resource(s);
+				if (bh.has_mines()) {
+					bo.mines_ = world.get_resource(bh.get_mines());
 				}
 
 				bo.mines_percent_ = bh.get_mines_percent();

=== modified file 'src/editor/ui_menus/editor_main_menu_save_map.cc'
--- src/editor/ui_menus/editor_main_menu_save_map.cc	2014-11-30 18:49:38 +0000
+++ src/editor/ui_menus/editor_main_menu_save_map.cc	2015-01-27 21:00:25 +0000
@@ -24,6 +24,7 @@
 #include <memory>
 #include <string>
 
+#include <boost/algorithm/string.hpp>
 #include <boost/format.hpp>
 
 #include "base/i18n.h"
@@ -358,15 +359,7 @@
 	g_fs->ensure_directory_exists(m_basedir);
 
 	//  OK, first check if the extension matches (ignoring case).
-	bool assign_extension = true;
-	if (filename.size() >= strlen(WLMF_SUFFIX)) {
-		char buffer[10]; //  enough for the extension
-		filename.copy
-			(buffer, sizeof(WLMF_SUFFIX), filename.size() - strlen(WLMF_SUFFIX));
-		if (!strncasecmp(buffer, WLMF_SUFFIX, strlen(WLMF_SUFFIX)))
-			assign_extension = false;
-	}
-	if (assign_extension)
+	if (!boost::iends_with(filename, WLMF_SUFFIX))
 		filename += WLMF_SUFFIX;
 
 	//  append directory name

=== modified file 'src/logic/building.cc'
--- src/logic/building.cc	2014-12-28 16:45:37 +0000
+++ src/logic/building.cc	2015-01-27 21:00:25 +0000
@@ -22,6 +22,7 @@
 #include <cstdio>
 #include <sstream>
 
+#include <boost/algorithm/string.hpp>
 #include <boost/format.hpp>
 
 #include "base/macros.h"
@@ -69,24 +70,26 @@
 	m_global        (false),
 	m_vision_range  (0)
 {
+	using boost::iequals;
+
 	try {
-		char const * const string = global_s.get_safe_string("size");
-		if      (!strcasecmp(string, "small"))
+		auto size = global_s.get_safe_string("size");
+		if      (iequals(size, "small"))
 			m_size = BaseImmovable::SMALL;
-		else if (!strcasecmp(string, "medium"))
+		else if (iequals(size, "medium"))
 			m_size = BaseImmovable::MEDIUM;
-		else if (!strcasecmp(string, "big"))
+		else if (iequals(size, "big"))
 			m_size = BaseImmovable::BIG;
-		else if (!strcasecmp(string, "mine")) {
+		else if (iequals(size, "mine")) {
 			m_size = BaseImmovable::SMALL;
 			m_mine = true;
-		} else if (!strcasecmp(string, "port")) {
+		} else if (iequals(size, "port")) {
 			m_size = BaseImmovable::BIG;
 			m_port = true;
 		} else
 			throw GameDataError
 				("expected %s but found \"%s\"",
-				 "{\"small\"|\"medium\"|\"big\"|\"port\"|\"mine\"}", string);
+				 "{\"small\"|\"medium\"|\"big\"|\"port\"|\"mine\"}", size);
 	} catch (const WException & e) {
 		throw GameDataError("size: %s", e.what());
 	}

=== modified file 'src/logic/immovable.cc'
--- src/logic/immovable.cc	2014-12-06 12:22:35 +0000
+++ src/logic/immovable.cc	2015-01-27 21:00:25 +0000
@@ -230,21 +230,25 @@
 	m_size          (BaseImmovable::NONE),
 	m_owner_tribe   (owner_tribe)
 {
-	if (char const * const string = global_s.get_string("size"))
+	using boost::iequals;
+
+	if (global_s.has_val("size")) {
+		auto size = global_s.get_string("size");
 		try {
-			if      (!strcasecmp(string, "small"))
+			if      (iequals(size, "small"))
 				m_size = BaseImmovable::SMALL;
-			else if (!strcasecmp(string, "medium"))
+			else if (iequals(size, "medium"))
 				m_size = BaseImmovable::MEDIUM;
-			else if (!strcasecmp(string, "big"))
+			else if (iequals(size, "big"))
 				m_size = BaseImmovable::BIG;
 			else
 				throw GameDataError
 					("expected %s but found \"%s\"",
-					 "{\"small\"|\"medium\"|\"big\"}", string);
+					 "{\"small\"|\"medium\"|\"big\"}", size);
 		} catch (const WException & e) {
 			throw GameDataError("size: %s", e.what());
 		}
+	}
 
 	// parse attributes
 	{

=== modified file 'src/logic/save_handler.cc'
--- src/logic/save_handler.cc	2015-01-12 15:44:52 +0000
+++ src/logic/save_handler.cc	2015-01-27 21:00:25 +0000
@@ -21,6 +21,7 @@
 
 #include <memory>
 
+#include <boost/algorithm/string.hpp>
 #include <boost/format.hpp>
 
 #include "base/log.h"
@@ -156,15 +157,7 @@
 	(std::string dir, std::string filename)
 {
 	// ok, first check if the extension matches (ignoring case)
-	bool assign_extension = true;
-	if (filename.size() >= strlen(WLGF_SUFFIX)) {
-		char buffer[10]; // enough for the extension
-		filename.copy
-			(buffer, sizeof(WLGF_SUFFIX), filename.size() - strlen(WLGF_SUFFIX));
-		if (!strncasecmp(buffer, WLGF_SUFFIX, strlen(WLGF_SUFFIX)))
-			assign_extension = false;
-	}
-	if (assign_extension)
+	if (!boost::iends_with(filename, WLGF_SUFFIX))
 		filename += WLGF_SUFFIX;
 
 	// Now append directory name

=== modified file 'src/map_io/widelands_map_loader.h'
--- src/map_io/widelands_map_loader.h	2014-09-10 08:55:04 +0000
+++ src/map_io/widelands_map_loader.h	2015-01-27 21:00:25 +0000
@@ -20,7 +20,7 @@
 #ifndef WL_MAP_IO_WIDELANDS_MAP_LOADER_H
 #define WL_MAP_IO_WIDELANDS_MAP_LOADER_H
 
-#include <cstring>
+#include <boost/algorithm/string.hpp>
 #include <memory>
 #include <string>
 
@@ -46,7 +46,7 @@
 	MapObjectLoader * get_map_object_loader() {return m_mol.get();}
 
 	static bool is_widelands_map(const std::string & filename) {
-		return !strcasecmp(&filename.c_str()[filename.size() - 4], WLMF_SUFFIX);
+		return boost::iends_with(filename, WLMF_SUFFIX);
 	}
 
 	// If this was made pre one-world, the name of the world.

=== modified file 'src/profile/profile.cc'
--- src/profile/profile.cc	2014-10-15 08:21:03 +0000
+++ src/profile/profile.cc	2015-01-27 21:00:25 +0000
@@ -19,13 +19,17 @@
 
 #include "profile/profile.h"
 
+#include <algorithm>
 #include <cctype>
 #include <cstdarg>
 #include <cstdio>
+#include <cstdlib>
 #include <cstring>
 #include <limits>
 #include <string>
 
+#include <boost/algorithm/string.hpp>
+
 #include "base/i18n.h"
 #include "base/log.h"
 #include "base/wexception.h"
@@ -74,28 +78,38 @@
 
 Profile g_options(Profile::err_log);
 
-Section::Value::Value(const char * const nname, const char * const nval) :
-	m_used(false), m_name(strdup(nname)), m_value(strdup(nval))
-{}
+Section::Value::Value(const string & nname, const char * const nval) :
+	m_used(false),
+	m_name(nname)
+{
+	set_string(nval);
+}
 
 Section::Value::Value(const Section::Value & o) :
-m_used(o.m_used), m_name(strdup(o.m_name)), m_value(strdup(o.m_value)) {}
-
-Section::Value::~Value()
-{
-	free(m_name);
-	free(m_value);
-}
-
-Section::Value & Section::Value::operator= (const Section::Value & o)
-{
-	if (this != &o) {
-		free(m_name);
-		free(m_value);
-		m_used = o.m_used;
-		m_name = strdup(o.m_name);
-		m_value = strdup(o.m_value);
-	}
+	m_used(o.m_used),
+	m_name(o.m_name)
+{
+	set_string(o.m_value.get());
+}
+
+Section::Value::Value(Section::Value && o)
+	: Value()
+{
+	using std::swap;
+	swap(*this, o);
+}
+
+Section::Value & Section::Value::operator= (Section::Value other)
+{
+	using std::swap;
+	swap(*this, other);
+	return *this;
+}
+
+Section::Value & Section::Value::operator= (Section::Value && other)
+{
+	using std::swap;
+	swap(*this, other);
 	return *this;
 }
 
@@ -112,12 +126,12 @@
 int32_t Section::Value::get_int() const
 {
 	char * endp;
-	long int const i = strtol(m_value, &endp, 0);
+	long int const i = strtol(m_value.get(), &endp, 0);
 	if (*endp)
-		throw wexception("%s: '%s' is not an integer", get_name(), m_value);
+		throw wexception("%s: '%s' is not an integer", get_name(), get_string());
 	int32_t const result = i;
 	if (i != result)
-		throw wexception("%s: '%s' is out of range",   get_name(), m_value);
+		throw wexception("%s: '%s' is out of range",   get_name(), get_string());
 
 	return result;
 }
@@ -126,9 +140,9 @@
 uint32_t Section::Value::get_natural() const
 {
 	char * endp;
-	long long int i = strtoll(m_value, &endp, 0);
+	long long int i = strtoll(m_value.get(), &endp, 0);
 	if (*endp || i < 0)
-		throw wexception("%s: '%s' is not natural", get_name(), m_value);
+		throw wexception("%s: '%s' is not natural", get_name(), get_string());
 	return i;
 }
 
@@ -136,9 +150,9 @@
 uint32_t Section::Value::get_positive() const
 {
 	char * endp;
-	long long int i = strtoll(m_value, &endp, 0);
+	long long int i = strtoll(m_value.get(), &endp, 0);
 	if (*endp || i < 1)
-		throw wexception("%s: '%s' is not positive", get_name(), m_value);
+		throw wexception("%s: '%s' is not positive", get_name(), get_string());
 	return i;
 }
 
@@ -146,31 +160,43 @@
 bool Section::Value::get_bool() const
 {
 	for (int32_t i = 0; i < TRUE_WORDS; ++i)
-		if (!strcasecmp(m_value, trueWords[i]))
+		if (boost::iequals(m_value.get(), trueWords[i]))
 			return true;
 	for (int32_t i = 0; i < FALSE_WORDS; ++i)
-		if (!strcasecmp(m_value, falseWords[i]))
+		if (boost::iequals(m_value.get(), falseWords[i]))
 			return false;
 
-	throw wexception("%s: '%s' is not a boolean value", get_name(), m_value);
+	throw wexception("%s: '%s' is not a boolean value", get_name(), get_string());
 }
 
 
 Point Section::Value::get_point() const
 {
-	char * endp = m_value;
+	char * endp = m_value.get();
 	long int const x = strtol(endp, &endp, 0);
 	long int const y = strtol(endp, &endp, 0);
 	if (*endp)
-		throw wexception("%s: '%s' is not a Point", get_name(), m_value);
+		throw wexception("%s: '%s' is not a Point", get_name(), get_string());
 
 	return Point(x, y);
 }
 
 void Section::Value::set_string(char const * const value)
 {
-	free(m_value);
-	m_value = strdup(value);
+	using std::copy;
+
+	auto len = strlen(value) + 1;
+	m_value.reset(new char[len]);
+	copy(value, value + len, m_value.get());
+}
+
+void swap(Section::Value & first, Section::Value & second)
+{
+	using std::swap;
+
+	swap(first.m_name,  second.m_name);
+	swap(first.m_value, second.m_value);
+	swap(first.m_used,  second.m_used);
 }
 
 
@@ -189,29 +215,9 @@
 	m_section_name = name;
 }
 
-Section::Section(Profile * const prof, const char * const name) :
+Section::Section(Profile * const prof, const std::string & name) :
 m_profile(prof), m_used(false), m_section_name(name) {}
 
-Section::Section(const Section & o) :
-	m_profile     (o.m_profile),
-	m_used        (o.m_used),
-	m_section_name(o.m_section_name),
-	m_values      (o.m_values)
-{
-	assert(this != &o);
-}
-
-Section & Section::operator= (const Section & o) {
-	if (this != &o) {
-		m_profile      = o.m_profile;
-		m_used         = o.m_used;
-		m_section_name = o.m_section_name;
-		m_values       = o.m_values;
-	}
-
-	return *this;
-}
-
 /** Section::is_used()
  *
  */
@@ -248,7 +254,7 @@
 bool Section::has_val(char const * const name) const
 {
 	for (const Value& temp_value : m_values) {
-		if (!strcasecmp(temp_value.get_name(), name)) {
+		if (boost::iequals(temp_value.get_name(), name)) {
 			return true;
 		}
 	}
@@ -265,7 +271,7 @@
 Section::Value * Section::get_val(char const * const name)
 {
 	for (Value& value : m_values) {
-		if (!strcasecmp(value.get_name(), name)) {
+		if (boost::iequals(value.get_name(), name)) {
 			value.mark_used();
 			return &value;
 		}
@@ -284,7 +290,7 @@
 {
 	for (Value& value : m_values) {
 		if (!value.is_used()) {
-			if (!name || !strcasecmp(value.get_name(), name)) {
+			if (!name || boost::iequals(value.get_name(), name)) {
 				value.mark_used();
 				return &value;
 			}
@@ -297,7 +303,7 @@
 	(char const * const name, char const * const value)
 {
 	for (Value& temp_value : m_values) {
-		if (!strcasecmp(temp_value.get_name(), name)) {
+		if (boost::iequals(temp_value.get_name(), name)) {
 			temp_value.set_string(value);
 			return temp_value;
 		}
@@ -308,8 +314,7 @@
 Section::Value & Section::create_val_duplicate
 	(char const * const name, char const * const value)
 {
-	Value v(name, value);
-	m_values.push_back(v);
+	m_values.emplace_back(name, value);
 	return m_values.back();
 }
 
@@ -617,7 +622,7 @@
 Section * Profile::get_section(const std::string & name)
 {
 	for (Section& temp_section : m_sections) {
-		if (!strcasecmp(temp_section.get_name(), name.c_str())) {
+		if (boost::iequals(temp_section.get_name(), name.c_str())) {
 			temp_section.mark_used();
 			return &temp_section;
 		}
@@ -661,7 +666,7 @@
 {
 	for (Section& section : m_sections) {
 		if (!section.is_used()) {
-			if (!name || !strcasecmp(section.get_name(), name)) {
+			if (!name || boost::iequals(section.get_name(), name)) {
 				section.mark_used();
 				return &section;
 			}
@@ -674,7 +679,7 @@
 Section & Profile::create_section          (char const * const name)
 {
 	for (Section& section : m_sections) {
-		if (!strcasecmp(section.get_name(), name)) {
+		if (boost::iequals(section.get_name(), name)) {
 			return section;
 		}
 	}

=== modified file 'src/profile/profile.h'
--- src/profile/profile.h	2014-09-20 09:37:47 +0000
+++ src/profile/profile.h	2015-01-27 21:00:25 +0000
@@ -21,6 +21,8 @@
 #define WL_PROFILE_PROFILE_H
 
 #include <cstring>
+#include <memory>
+#include <string>
 #include <vector>
 
 #include "base/macros.h"
@@ -55,17 +57,18 @@
 	friend class Profile;
 
 	struct Value {
-		bool   m_used;
-		char * m_name;
-		char * m_value;
+		using string = std::string;
 
-		Value(char const * nname, char const * nval);
+		Value(const string & name, const char * const value);
 		Value(const Value &);
-		~Value();
-
-		Value & operator= (const Value &);
-
-		char const * get_name() const {return m_name;}
+		Value(Value && other);
+
+		// destructor would be empty
+
+		Value & operator= (Value);
+		Value & operator= (Value && other);
+
+		char const * get_name() const {return m_name.c_str();}
 
 		bool is_used() const;
 		void mark_used();
@@ -74,19 +77,25 @@
 		uint32_t get_natural () const;
 		uint32_t get_positive() const;
 		bool get_bool() const;
-		char const * get_string() const {return m_value;}
-		char       * get_string()       {return m_value;}
+		char const * get_string() const {return m_value.get();}
+		char       * get_string()       {return m_value.get();}
 		Point  get_point () const;
 
 		void set_string(char const *);
+
+		friend void swap(Value& first, Value& second);
+
+	private:
+		bool m_used;
+		string m_name;
+		std::unique_ptr<char []> m_value;
+
+		Value() = default;
 	};
 
 	using ValueList = std::vector<Value>;
 
-	Section(Profile *, char const * name);
-	Section(const Section &);
-
-	Section & operator= (const Section &);
+	Section(Profile *, const std::string & name);
 
 	/// \returns whether a value with the given name exists.
 	/// Does not mark the value as used.