Skip to content

Commit

Permalink
Unit: refactor upkeep handling to use a visitor pattern
Browse files Browse the repository at this point in the history
  • Loading branch information
Vultraz committed Apr 14, 2017
1 parent ade221e commit ebeeae3
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 31 deletions.
11 changes: 6 additions & 5 deletions src/scripting/lua_unit.cpp
Expand Up @@ -300,13 +300,14 @@ static int impl_unit_get(lua_State *L)

if(strcmp(m, "upkeep") == 0) {
unit::upkeep_t upkeep = u.upkeep_raw();
if(boost::get<unit::upkeep_full>(&upkeep) != nullptr){
lua_pushstring(L, "full");
} else if(boost::get<unit::upkeep_loyal>(&upkeep) != nullptr){
lua_pushstring(L, "loyal");

if(int* v = boost::get<int>(&upkeep)) {
lua_push(L, *v);
} else {
lua_push(L, boost::get<int>(upkeep));
const std::string type = boost::apply_visitor(unit::upkeep_type_visitor(), upkeep);
lua_pushstring(L, type.c_str());

This comment has been minimized.

Copy link
@CelticMinstrel

CelticMinstrel Apr 14, 2017

Member

lua_pushstring is for pushing C strings. Use lua_pushlstring for C++ strings.

This comment has been minimized.

Copy link
@gfgtdf

gfgtdf Apr 14, 2017

Contributor

this might be an impovement, but note that the other code (game_lua_kernel.cpp) also uses lua_pushstring to push c++ strings in >90% of cases.

This comment has been minimized.

Copy link
@gfgtdf

gfgtdf Apr 14, 2017

Contributor

actually lua_push from push_check.hpp has an overload that takes std::string and migth be easier than using lua_pushlstring directly, (not here but in cases where the second parameter is a temporary)

This comment has been minimized.

Copy link
@CelticMinstrel

CelticMinstrel Apr 14, 2017

Member

Yes, lua_pushstring does work in the average case, and will certainly work here (since this is an average case), but I prefer to use lua_pushlstring everywhere unless it's actually a C string.

}

return 1;
}
if(strcmp(m, "advancements") == 0) {
Expand Down
28 changes: 7 additions & 21 deletions src/units/unit.cpp
Expand Up @@ -1467,15 +1467,8 @@ int unit::upkeep() const
if(can_recruit()) {
return 0;
}
else if(boost::get<upkeep_full>(&upkeep_) != nullptr) {
return level();
}
else if(boost::get<upkeep_loyal>(&upkeep_) != nullptr) {
return 0;
}
else {
return boost::get<int>(upkeep_);
}

return boost::apply_visitor(upkeep_value_visitor(*this), upkeep_);
}

bool unit::loyal() const
Expand Down Expand Up @@ -1894,7 +1887,7 @@ void unit::apply_builtin_effect(std::string apply_to, const config& effect)
if(increase.empty() == false) {
max_experience_ = utils::apply_modifier(max_experience_, increase, 1);
}
} else if(apply_to == "loyal") {
} else if(apply_to == upkeep_loyal::type()) {
upkeep_ = upkeep_loyal();;
} else if(apply_to == "status") {
const std::string& add = effect["add"];
Expand Down Expand Up @@ -2429,28 +2422,21 @@ void unit::parse_upkeep(const config::attribute_value& upkeep)
if(upkeep_int != -99) {
upkeep_ = upkeep_int;
}
else if(upkeep == "loyal" || upkeep == "free") {
else if(upkeep == upkeep_loyal::type() || upkeep == "free") {
upkeep_ = upkeep_loyal();
}
else if(upkeep == "full") {
else if(upkeep == upkeep_full::type()) {
upkeep_ = upkeep_full();
}
else {
WRN_UT << "Fund invalid upkeep=\"" << upkeep << "\" in a unit\n";
upkeep_ = upkeep_full();
}
}

void unit::write_upkeep(config::attribute_value& upkeep) const
{
if(boost::get<upkeep_full>(&upkeep_) != nullptr) {
upkeep = "full";
}
else if(boost::get<upkeep_loyal>(&upkeep_) != nullptr) {
upkeep = "loyal";
}
else {
upkeep = boost::get<int>(upkeep_);
}
upkeep = boost::apply_visitor(upkeep_type_visitor(), upkeep_);
}

// Filters unimportant stats from the unit config and returns a checksum of
Expand Down
58 changes: 53 additions & 5 deletions src/units/unit.hpp
Expand Up @@ -740,10 +740,58 @@ class unit
*/
int upkeep() const;

// TODO: look for a cleaner solution than these structs
struct upkeep_full {};
struct upkeep_loyal {};
using upkeep_t = boost::variant<upkeep_full, upkeep_loyal, int>;
struct upkeep_full
{
static std::string type() { static std::string v = "full"; return v; }

This comment has been minimized.

Copy link
@CelticMinstrel

CelticMinstrel Apr 14, 2017

Member

Personally, I'd use const char* for these... I guess it doesn't really matter though...

};

struct upkeep_loyal
{
static std::string type() { static std::string v = "loyal"; return v; }
};

/** Visitor helper class to fetch the appropriate upkeep value. */
class upkeep_value_visitor : public boost::static_visitor<int>
{
public:
explicit upkeep_value_visitor(const unit& unit) : u_(unit) {}

int operator()(const upkeep_full&) const
{
return u_.level();
}

int operator()(const upkeep_loyal&) const
{
return 0;
}

int operator()(int v) const
{
return v;
}

private:
const unit& u_;
};

/** Visitor helper struct to fetch the upkeep type flag if applicable, or the the value otherwise. */
struct upkeep_type_visitor : public boost::static_visitor<std::string>
{
template<typename T>
typename std::enable_if<!std::is_same<int, T>::value, std::string>::type
operator()(T& v) const
{
return v.type();
}

std::string operator()(int v) const
{
return std::to_string(v);
}
};

using upkeep_t = boost::variant<upkeep_full, upkeep_loyal, int>;

/**
* Gets the raw variant controlling the upkeep value.
Expand All @@ -755,7 +803,7 @@ class unit
return upkeep_;
}

/** Sets the upkeep value to a specific numeric value. */
/** Sets the upkeep value to a specific value value. Does not necessarily need to be numeric */
void set_upkeep(upkeep_t v)
{
upkeep_ = v;
Expand Down

0 comments on commit ebeeae3

Please sign in to comment.