Address a bunch of critique from the feedback Minstrel

src/formula/variant.cpp

@@ -192,9 +192,9 @@ variant_iterator& variant_iterator::operator=(const variant_iterator& that)
 bool variant_iterator::operator==(const variant_iterator& that) const
 {
 	if(type_ == TYPE_LIST) {
-		return that.type_ != TYPE_LIST ? false : list_iterator_ == that.list_iterator_;
+		return that.type_ == TYPE_LIST && list_iterator_ == that.list_iterator_;
 	} else if(type_ == TYPE_MAP) {
-		return that.type_ != TYPE_MAP  ? false : map_iterator_ == that.map_iterator_;
+		return that.type_ == TYPE_MAP && map_iterator_ == that.map_iterator_;
 	} else if(type_== TYPE_NULL && that.type_ == TYPE_NULL) {
 		return true;
 	}
@@ -254,11 +254,6 @@ variant::variant(const std::map<variant,variant>& map)
 	assert(value_.get());
 }
 
-variant::variant(const variant& v)
-    : value_(v.value_)
-{
-}
-
 variant& variant::operator=(const variant& v)
 {
 	value_ = v.value_;
@@ -372,6 +367,12 @@ bool variant::is_empty() const
 
 size_t variant::num_elements() const
 {
+	if(!is_list() && !is_map()) {
+		throw type_error(formatter() << "type error: "
+			<< " expected a list or a map but found " << type_string()
+			<< " (" << to_debug_string() << ")");
+	}
+
 	return value_->num_elements();
 }
 
@@ -682,7 +683,7 @@ variant variant::concatenate(const variant& v) const
 		return variant(res);
 	} else {
 		throw type_error(formatter() << "type error: expected two "
-			<< " lists or two maps  but found " << type_string()
+			<< " lists or two maps but found " << type_string()
 			<< " (" << to_debug_string() << ")"
 			<< " and " << v.type_string()
 			<< " (" << v.to_debug_string() << ")");
@@ -698,15 +699,13 @@ variant variant::build_range(const variant& v) const
 
 bool variant::contains(const variant& v) const
 {
-	if(type() != VARIANT_TYPE::TYPE_LIST && type() != VARIANT_TYPE::TYPE_MAP) {
-		throw type_error(formatter() << "type error: expected "
-			<< variant_type_to_string(VARIANT_TYPE::TYPE_LIST) << " or "
-			<< variant_type_to_string(VARIANT_TYPE::TYPE_MAP) << " but found "
-			<< type_string()
+	if(!is_list() && !is_map()) {
+		throw type_error(formatter() << "type error: "
+			<< " expected a list or a map but found " << type_string()
 			<< " (" << to_debug_string() << ")");
 	}
 
-	if(type() == VARIANT_TYPE::TYPE_LIST) {
+	if(is_list()) {
 		return value_cast<game_logic::variant_list>()->contains(v);
 	} else {
 		return value_cast<game_logic::variant_map>()->contains(v);
@@ -725,7 +724,7 @@ void variant::must_be(VARIANT_TYPE t) const
 void variant::must_both_be(VARIANT_TYPE t, const variant& second) const
 {
 	if(type() != t || second.type() != t) {
-		throw type_error(formatter() << "type error: expected "
+		throw type_error(formatter() << "type error: expected two "
 			<< variant_type_to_string(t) << " but found "
 			<<        type_string() << " (" <<        to_debug_string() << ")" << " and "
 			<< second.type_string() << " (" << second.to_debug_string() << ")");

src/formula/variant.hpp

@@ -59,7 +59,6 @@ class variant
 	explicit variant(const std::string& str);
 	explicit variant(const std::map<variant, variant>& map);
 
-	variant(const variant& v);
 	variant& operator=(const variant& v);
 
 	variant operator[](size_t n) const;
@@ -149,9 +148,6 @@ class variant
 	variant_iterator begin() const;
 	variant_iterator end() const;
 
-	//auto begin()->decltype(value_cast<game_logic::variant_callable>()->get_iter());
-	//auto end()->decltype(value_cast<game_logic::variant_callable>()->get_iter());
-
 	std::string serialize_to_string() const;
 	void serialize_from_string(const std::string& str);
 
@@ -183,7 +179,7 @@ class variant
 
 	/**
 	 * Variant value.
-	 * Each of the constructors casts this to an appropriate helper class.
+	 * Each of the constructors initialized this with the appropriate helper class.
 	 */
 	game_logic::value_base_ptr value_;
 };

src/formula/variant_private.hpp

@@ -54,9 +54,7 @@ class variant_value_base;
 
 using value_base_ptr = std::shared_ptr<variant_value_base>;
 
-/**
- * Helper functions to cast a @ref variant_value_base to a new derived type.
- */
+/** Casts a @ref variant_value_base shared pointer to a new derived type. */
 template<typename T>
 static std::shared_ptr<T> value_cast(value_base_ptr ptr)
 {
@@ -68,6 +66,7 @@ static std::shared_ptr<T> value_cast(value_base_ptr ptr)
 	return res;
 }
 
+/** Casts a @ref variant_value_base reference to a new derived type. */
 template<typename T>
 static T& value_ref_cast(variant_value_base& ptr)
 {
@@ -376,7 +375,6 @@ class variant_container : public virtual variant_value_base
 public:
 	explicit variant_container(const T& container)
 		: container_(container)
-		, container_iter_(container_.begin())
 	{
 		// NOTE: add more conditions if this changes.
 		static_assert((std::is_same<variant_vector, T>::value || std::is_same<variant_map_raw, T>::value),
@@ -434,7 +432,6 @@ class variant_container : public virtual variant_value_base
 	std::string to_string_impl(bool annotate, bool annotate_empty, mod_func_t mod_func) const;
 
 	T container_;
-	typename T::iterator container_iter_;
 };