Skip to content

Commit

Permalink
Use !std::string::empty() for string-is-not-empty comparisons
Browse files Browse the repository at this point in the history
  • Loading branch information
Vultraz committed Sep 4, 2017
1 parent 23e6863 commit 2eacb4e
Show file tree
Hide file tree
Showing 25 changed files with 45 additions and 45 deletions.
4 changes: 2 additions & 2 deletions src/arrow.cpp
Expand Up @@ -266,12 +266,12 @@ void arrow::update_symbols()
}

image_filename = dirname + style_ + "/" + prefix;
if (suffix != "")
if (!suffix.empty())
{
image_filename += ("-" + suffix);
}
image_filename += ".png";
assert(image_filename != "");
assert(!image_filename.empty());

image::locator image = image::locator(image_filename, mods);
if (!image.file_exists())
Expand Down
2 changes: 1 addition & 1 deletion src/display.cpp
Expand Up @@ -1624,7 +1624,7 @@ void display::set_diagnostic(const std::string& msg)
diagnostic_label_ = 0;
}

if(msg != "") {
if(!msg.empty()) {
font::floating_label flabel(msg);
flabel.set_font_size(font::SIZE_PLUS);
flabel.set_color(font::YELLOW_COLOR);
Expand Down
2 changes: 1 addition & 1 deletion src/floating_textbox.cpp
Expand Up @@ -113,7 +113,7 @@ namespace gui{
label_string_ = label;
mode_ = mode;

if(check_label != "") {
if(!check_label.empty()) {
check_.reset(new gui::button(gui.video(),check_label,gui::button::TYPE_CHECK));
check_->set_check(checked);
}
Expand Down
2 changes: 1 addition & 1 deletion src/game_initialization/create_engine.cpp
Expand Up @@ -392,7 +392,7 @@ void create_engine::prepare_for_campaign(const std::string& difficulty)
{
DBG_MP << "preparing data for campaign by reloading game config\n";

if(difficulty != "") {
if(!difficulty.empty()) {
state_.classification().difficulty = difficulty;
} else if(!selected_campaign_difficulty_.empty()) {
state_.classification().difficulty = selected_campaign_difficulty_;
Expand Down
4 changes: 2 additions & 2 deletions src/game_launcher.cpp
Expand Up @@ -850,7 +850,7 @@ bool game_launcher::play_multiplayer(mp_selection res)
} catch (mapgen_exception& e) {
gui2::show_error_message(video(), _("Map generator error: ") + e.message);
} catch(wesnothd_error& e) {
if(e.message != "") {
if(!e.message.empty()) {
ERR_NET << "caught network error: " << e.message << std::endl;
gui2::show_transient_message(video()
, ""
Expand All @@ -859,7 +859,7 @@ bool game_launcher::play_multiplayer(mp_selection res)
ERR_NET << "caught network error" << std::endl;
}
} catch(config::error& e) {
if(e.message != "") {
if(!e.message.empty()) {
ERR_CONFIG << "caught config::error: " << e.message << std::endl;
gui2::show_transient_message(video(), "", e.message);
} else {
Expand Down
2 changes: 1 addition & 1 deletion src/generators/cave_map_generator.cpp
Expand Up @@ -308,7 +308,7 @@ double passage_path_calculator::cost(const map_location& loc, const double) cons
void cave_map_generator::cave_map_generator_job::place_passage(const passage& p)
{
const std::string& chance = p.cfg["chance"];
if(chance != "" && int(rng_()%100) < std::stoi(chance)) {
if(!chance.empty() && int(rng_()%100) < std::stoi(chance)) {
return;
}

Expand Down
6 changes: 3 additions & 3 deletions src/generators/default_map_generator_job.cpp
Expand Up @@ -168,7 +168,7 @@ namespace {
to(t_translation::GRASS_LAND)
{
const std::string& terrain = cfg["terrain"];
if(terrain != "") {
if(!terrain.empty()) {
to = t_translation::read_terrain_code(terrain);
}
}
Expand Down Expand Up @@ -207,7 +207,7 @@ namespace {
, to(t_translation::NONE_TERRAIN)
{
const std::string& to_str = cfg["to"];
if(to_str != "") {
if(!to_str.empty()) {
to = t_translation::read_terrain_code(to_str);
}
}
Expand Down Expand Up @@ -1116,7 +1116,7 @@ std::string default_map_generator_job::default_generate_map(generator_data data,
terrain[x][y] = letter;
if(misc_labels != nullptr) {
const map_location loc(x - data.width / 3, y - data.height / 3); //add to use for village naming
if(road_base_name != "")
if(!road_base_name.empty())
road_names.emplace(loc, road_base_name);
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/gui/dialogs/campaign_selection.cpp
Expand Up @@ -95,7 +95,7 @@ void campaign_selection::campaign_selected(window& window)
}

assert(tree.selected_item());
if(tree.selected_item()->id() != "") {
if(!tree.selected_item()->id().empty()) {
auto iter = std::find(page_ids_.begin(), page_ids_.end(), tree.selected_item()->id());
const int choice = iter - page_ids_.begin();
if(iter == page_ids_.end()) {
Expand Down Expand Up @@ -296,7 +296,7 @@ void campaign_selection::post_show(window& window)
}

assert(tree.selected_item());
if(tree.selected_item()->id() != "") {
if(!tree.selected_item()->id().empty()) {
auto iter = std::find(page_ids_.begin(), page_ids_.end(), tree.selected_item()->id());
if(iter != page_ids_.end()) {
choice_ = iter - page_ids_.begin();
Expand Down
2 changes: 1 addition & 1 deletion src/help/help.cpp
Expand Up @@ -206,7 +206,7 @@ void show_help(CVideo& video, const section &toplevel_sec,
hb.set_location(xloc + left_padding, yloc + top_padding);
hb.set_width(width - left_padding - right_padding);
hb.set_height(height - top_padding - bot_padding);
if (show_topic != "") {
if (!show_topic.empty()) {
hb.show_topic(show_topic);
}
else {
Expand Down
4 changes: 2 additions & 2 deletions src/help/help_browser.cpp
Expand Up @@ -145,7 +145,7 @@ void help_browser::handle_event(const SDL_Event &event)
const int mousex = mouse_event.x;
const int mousey = mouse_event.y;
const std::string ref = text_area_.ref_at(mousex, mousey);
if (ref != "") {
if (!ref.empty()) {
const topic *t = find_topic(toplevel_, ref);
if (t == nullptr) {
std::stringstream msg;
Expand All @@ -170,7 +170,7 @@ void help_browser::update_cursor()
int mousex, mousey;
SDL_GetMouseState(&mousex,&mousey);
const std::string ref = text_area_.ref_at(mousex, mousey);
if (ref != "" && !ref_cursor_) {
if (!ref.empty() && !ref_cursor_) {
cursor::set(cursor::HYPERLINK);
ref_cursor_ = true;
}
Expand Down
8 changes: 4 additions & 4 deletions src/help/help_impl.cpp
Expand Up @@ -1245,7 +1245,7 @@ std::vector<std::string> parse_text(const std::string &text)
msg << "Element '" << ss.str() << "' continues through end of string.";
throw parse_error(msg.str());
}
if (ss.str() != "") {
if (!ss.str().empty()) {
// Add the last string.
res.push_back(ss.str());
}
Expand Down Expand Up @@ -1289,7 +1289,7 @@ std::string convert_to_wml(const std::string &element_name, const std::string &c
msg << "Unterminated single quote after: '" << ss.str() << "'";
throw parse_error(msg.str());
}
if (ss.str() != "") {
if (!ss.str().empty()) {
attributes.push_back(ss.str());
}
ss.str("");
Expand Down Expand Up @@ -1407,7 +1407,7 @@ void generate_contents()
// toplevel. Hence, add it to the hidden ones if it
// is not referenced from another section.
if (!section_is_referenced(id, *help_config)) {
if (ss.str() != "") {
if (!ss.str().empty()) {
ss << ",";
}
ss << id;
Expand All @@ -1421,7 +1421,7 @@ void generate_contents()
const std::string id = topic["id"];
if (find_topic(default_toplevel, id) == nullptr) {
if (!topic_is_referenced(id, *help_config)) {
if (ss.str() != "") {
if (!ss.str().empty()) {
ss << ",";
}
ss << id;
Expand Down
8 changes: 4 additions & 4 deletions src/help/help_text_area.cpp
Expand Up @@ -116,7 +116,7 @@ void help_text_area::set_items()
std::vector<std::string> const &parsed_items = shown_topic_->text.parsed_text();
std::vector<std::string>::const_iterator it;
for (it = parsed_items.begin(); it != parsed_items.end(); ++it) {
if (*it != "" && (*it)[0] == '[') {
if (!(*it).empty() && (*it)[0] == '[') {
// Should be parsed as WML.
try {
config cfg;
Expand Down Expand Up @@ -247,7 +247,7 @@ void help_text_area::handle_jump_cfg(const config &cfg)
throw parse_error("Jump markup must have either a to or an amount attribute.");
}
unsigned jump_to = curr_loc_.first;
if (amount_str != "") {
if (!amount_str.empty()) {
unsigned amount;
try {
amount = lexical_cast<unsigned, std::string>(amount_str);
Expand All @@ -257,7 +257,7 @@ void help_text_area::handle_jump_cfg(const config &cfg)
}
jump_to += amount;
}
if (to_str != "") {
if (!to_str.empty()) {
unsigned to;
try {
to = lexical_cast<unsigned, std::string>(to_str);
Expand Down Expand Up @@ -577,7 +577,7 @@ std::string help_text_area::ref_at(const int x, const int y)
const std::list<item>::const_iterator it =
std::find_if(items_.begin(), items_.end(), item_at(local_x, cmp_y));
if (it != items_.end()) {
if ((*it).ref_to != "") {
if (!(*it).ref_to.empty()) {
return ((*it).ref_to);
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/hotkey/hotkey_item.hpp
Expand Up @@ -264,7 +264,7 @@ class hotkey_keyboard: public hotkey_base
*/
virtual bool valid() const
{
return keycode_ != SDLK_UNKNOWN && text_ != "";
return keycode_ != SDLK_UNKNOWN && !text_.empty();
}

protected:
Expand Down
2 changes: 1 addition & 1 deletion src/map/label.cpp
Expand Up @@ -118,7 +118,7 @@ const terrain_label* map_labels::get_label(const map_location& loc) const

// No such team label. Try to find global label, except if that's what we just did.
// NOTE: This also avoid infinite recursion
if(res == nullptr && team_name() != "") {
if(res == nullptr && !team_name().empty()) {
return get_label(loc, "");
}

Expand Down
2 changes: 1 addition & 1 deletion src/playsingle_controller.cpp
Expand Up @@ -155,7 +155,7 @@ void playsingle_controller::play_scenario_init()
saved_game_.replay_start() = to_config();
}
start_game();
if( saved_game_.classification().random_mode != "" && is_networked_mp()) {
if(!saved_game_.classification().random_mode.empty() && is_networked_mp()) {
// This won't cause errors later but we should notify the user about it in case he didn't knew it.
gui2::show_transient_message(
gui_->video(),
Expand Down
4 changes: 2 additions & 2 deletions src/saved_game.cpp
Expand Up @@ -309,9 +309,9 @@ void saved_game::expand_mp_events()
boost::copy( mp_settings_.active_mods
| boost::adaptors::transformed(modevents_entry_for("modification"))
, std::back_inserter(mods) );
if(mp_settings_.mp_era != "") //We don't want the error message below if there is no era (= if this is a sp game)
if(!mp_settings_.mp_era.empty()) //We don't want the error message below if there is no era (= if this is a sp game)
{ mods.emplace_back("era", mp_settings_.mp_era); }
if(classification_.campaign != "")
if(!classification_.campaign.empty())
{ mods.emplace_back("campaign", classification_.campaign); }

// In the first iteration mod contains no [resource]s in all other iterations, mods contains only [resource]s
Expand Down
2 changes: 1 addition & 1 deletion src/serialization/string_utils.cpp
Expand Up @@ -597,7 +597,7 @@ std::string si_string(double input, bool base2, const std::string& unit) {
si_string_impl_stream_write(ss, input);
ss << ' '
<< *prefix
<< (base2 && (*prefix != "") ? _("infix_binary^i") : "")
<< (base2 && (!(*prefix).empty()) ? _("infix_binary^i") : "")
<< unit;
return ss.str();
}
Expand Down
2 changes: 1 addition & 1 deletion src/server/game.cpp
Expand Up @@ -915,7 +915,7 @@ bool game::process_turn(simple_wml::document& data, const socket_ptr user) {
marked.push_back(index - marked.size());
} else if ((**command).child("speak")) {
simple_wml::node& speak = *(**command).child("speak");
if (speak["to_sides"] != "" || is_muted_observer(user)) {
if (!speak["to_sides"].empty() || is_muted_observer(user)) {
DBG_GAME << "repackaging..." << std::endl;
repackage = true;
}
Expand Down
4 changes: 2 additions & 2 deletions src/server/server.cpp
Expand Up @@ -814,7 +814,7 @@ void server::add_player(socket_ptr socket, const wesnothd::player& player)

send_to_player(socket, games_and_users_list_);

if (motd_ != "") {
if (!motd_.empty()) {
send_server_message(socket, motd_);
}

Expand Down Expand Up @@ -2599,7 +2599,7 @@ void server::motd_handler(const std::string& /*issuer_name*/, const std::string&
assert(out != NULL);

if (parameters == "") {
if (motd_ != "") {
if (!motd_.empty()) {
*out << "Message of the day:\n" << motd_;
return;
} else {
Expand Down
4 changes: 2 additions & 2 deletions src/tests/test_config.cpp
Expand Up @@ -151,11 +151,11 @@ BOOST_AUTO_TEST_CASE ( test_config_attribute_value )

// blank != "" test.
c = config();
BOOST_CHECK(cc["x"] != "");
BOOST_CHECK(!cc["x"].empty());
BOOST_CHECK(cc["x"].empty());
BOOST_CHECK(cc["x"].blank());

BOOST_CHECK(c["x"] != "");
BOOST_CHECK(!c["x"].empty());
BOOST_CHECK(c["x"].empty());
BOOST_CHECK(c["x"].blank());

Expand Down
10 changes: 5 additions & 5 deletions src/units/animation.cpp
Expand Up @@ -89,23 +89,23 @@ struct animation_cursor
const std::string s_branch_value = branch.attributes["value"];
const std::string s_branch_value_2nd = branch.attributes["value_second"];

if(s_branch_hits != "" && s_branch_hits == s_cfg_hits) {
if(!s_branch_hits.empty() && s_branch_hits == s_cfg_hits) {
previously_hits_set = true;
}

if(s_branch_direction != "" && s_branch_direction == s_cfg_direction) {
if(!s_branch_direction.empty() && s_branch_direction == s_cfg_direction) {
previously_direction_set = true;
}

if(s_branch_terrain != "" && s_branch_terrain == s_cfg_terrain) {
if(!s_branch_terrain.empty() && s_branch_terrain == s_cfg_terrain) {
previously_terrain_set = true;
}

if(s_branch_value != "" && s_branch_value == s_cfg_value) {
if(!s_branch_value.empty() && s_branch_value == s_cfg_value) {
previously_value_set = true;
}

if(s_branch_value_2nd != "" && s_branch_value_2nd == s_cfg_value_2nd) {
if(!s_branch_value_2nd.empty() && s_branch_value_2nd == s_cfg_value_2nd) {
previously_value_2nd_set = true;
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/units/attack_type.cpp
Expand Up @@ -65,7 +65,7 @@ attack_type::attack_type(const config& cfg) :
description_ = translation::egettext(id_.c_str());

if(icon_.empty()){
if (id_ != "")
if (!id_.empty())
icon_ = "attacks/" + id_ + ".png";
else
icon_ = "attacks/blank-attack.png";
Expand Down
2 changes: 1 addition & 1 deletion src/units/types.cpp
Expand Up @@ -391,7 +391,7 @@ void unit_type::build_created(
}

const std::string& advances_to_val = cfg_["advances_to"];
if(advances_to_val != "null" && advances_to_val != "") {
if(advances_to_val != "null" && !advances_to_val.empty()) {
advances_to_ = utils::split(advances_to_val);
}

Expand Down
4 changes: 2 additions & 2 deletions src/units/unit.cpp
Expand Up @@ -515,7 +515,7 @@ unit::unit(const config& cfg, bool use_traits, const vconfig* vcfg)
std::vector<std::string> temp_advances = utils::split(cfg["advances_to"]);
if(temp_advances.size() == 1 && temp_advances.front() == "null") {
advances_to_.clear();
} else if(temp_advances.size() >= 1 && temp_advances.front() != "") {
} else if(temp_advances.size() >= 1 && !temp_advances.front().empty()) {
advances_to_ = temp_advances;
}

Expand Down Expand Up @@ -1759,7 +1759,7 @@ std::string unit::describe_builtin_effect(std::string apply_to, const config& ef
std::string desc;
for(attack_ptr a : attacks_) {
bool affected = a->describe_modification(effect, &desc);
if(affected && desc != "") {
if(affected && !desc.empty()) {
attack_names.emplace_back(a->name(), "wesnoth-units");
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/widgets/widget.cpp
Expand Up @@ -317,7 +317,7 @@ void widget::set_tooltip_string(const std::string& str)
void widget::process_help_string(int mousex, int mousey)
{
if (!hidden() && sdl::point_in_rect(mousex, mousey, rect_)) {
if(help_string_ == 0 && help_text_ != "") {
if(help_string_ == 0 && !help_text_.empty()) {
//std::cerr << "setting help string to '" << help_text_ << "'\n";
help_string_ = video().set_help_string(help_text_);
}
Expand Down

6 comments on commit 2eacb4e

@jyrkive
Copy link
Member

@jyrkive jyrkive commented on 2eacb4e Sep 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me just say that I prefer comparison to "", it's more intuitive in my opinion.

@CelticMinstrel
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to use .empty() over the comparison, but don't really have a strong preference.

@gfgtdf
Copy link
Contributor

@gfgtdf gfgtdf commented on 2eacb4e Sep 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "" comparision is more obvious to understand, but i think .empty() might be faster? Not sure though.

@Ja-MiT
Copy link
Contributor

@Ja-MiT Ja-MiT commented on 2eacb4e Sep 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .empty() method is intended to be the fastest way to do this check. That is not to say that another method (like comparing to an empty string) might be just as fast -- or perhaps imperceptibly slower -- but to have the best shot at optimal behavior, the .empty() method is the way to go.

I think that the "more obvious" and "more intuitive" claims are based more on experience than anything else. We're used to seeing comparisons to the empty string. I'd recommend giving the change an honest chance and see if you change your mind about being less obvious/intuitive after seeing the other approach for a few months or so.

@GregoryLundberg
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The advantage of .empty() is it is unambiguous in the face of Unicode. Consider, there are zero-width codepoints such that ("" != "") where one is an empty string and the other a zero-width strung.

@CelticMinstrel
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding speed, I wouldn't expect .empty() to be any faster than comparing to an empty string. As a first step in a lexicographical comparison, you would usually compare the lengths. If they are of different lengths, they cannot be equal. That does make it implementation-dependent though, whereas .empty() should be equally fast on any platform with any C++ standard library (ie, whether you choose msvcprt, libstdc++, libc++, or any other versions that might exist).

Please sign in to comment.