Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Misc cleanups #6570

Merged
merged 5 commits into from Mar 22, 2022
Merged

Misc cleanups #6570

merged 5 commits into from Mar 22, 2022

Conversation

Vultraz
Copy link
Member

@Vultraz Vultraz commented Mar 22, 2022

For CI.

@github-actions github-actions bot added AI Issues with the AI engine, including micro AIs. Lua API Issues with the Lua engine and API. Terrain Issues that involve terrain definitions or their implementation in the engine. UI User interface issues, including both back-end and front-end issues. Units Issues that involve unit definitions or their implementation in the engine. labels Mar 22, 2022
@Vultraz Vultraz merged commit e95afda into master Mar 22, 2022
@Vultraz Vultraz deleted the misc-cleanups branch March 22, 2022 15:26
@@ -87,7 +87,7 @@ namespace
#ifdef CAIRO_HAS_WIN32_FONT
bool is_valid_font_file(const std::string& file)
{
static const std::array<std::string, 3> font_exts { ".ttf", ".ttc", ".otf" };
static const std::array font_exts { ".ttf", ".ttc", ".otf" };
Copy link
Member

Choose a reason for hiding this comment

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

No idea if it's a problem. but this change means three std::string values are constructed every time this function is called.

Copy link
Member

Choose a reason for hiding this comment

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

Using auto two lines down should help, depending on the API of filesystem::ends_with. Or append s to each of the constants.

@@ -106,7 +106,7 @@ static void verify(const unit_map& units, const config& cfg) {
u->write(u_cfg);

bool is_ok = true;
static const std::array<std::string, 4> fields {{"type","hitpoints","experience","side"}};
static const std::array fields {"type","hitpoints","experience","side"};
Copy link
Member

Choose a reason for hiding this comment

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

Again you've changed the type from std::string to const char*. Same as previous comment.

@@ -4726,7 +4726,7 @@ void game_lua_kernel::set_game_display(game_display * gd) {
* elsewhere (in the C++ code).
* Any child tags not in this list will be passed to Lua's on_load event.
*/
static const std::array<std::string, 24> handled_file_tags {{
static const std::array handled_file_tags {
Copy link
Member

Choose a reason for hiding this comment

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

Same thing again.

(Aside: It seems to me that the function using this could just be an std::find call.)

@@ -57,52 +57,52 @@ BOOST_AUTO_TEST_CASE( utils_split_test )

{
auto split = utils::split(test_string);
std::array<std::string, 7> expect = {"a", "bb", "ccc || d", "ee", "fff | | g", "hh", "iii"};
std::array expect = {"a", "bb", "ccc || d", "ee", "fff | | g", "hh", "iii"};
Copy link
Member

Choose a reason for hiding this comment

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

This probably no longer compiles because of the type change. My suggestion here is to just append s to each constant. (Note: use of that literal suffix does require using namespace std::literals; if I recall correctly.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Other tests already did the same thing without the literal.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I misread the code. Sorry about that. This one is fine.

#define ENUM_AND_ARRAY(...) \
enum class type { __VA_ARGS__ }; \
\
/** Provide a variable template for an array of matching size. */ \
Copy link
Member

Choose a reason for hiding this comment

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

This is not a variable template. A variable template is like template<typename T> bool whatever = something<T>::value;. Please fix the comment.

Also, why remove count? You could leave the count declaration and just use it here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, you're right, I meant alias template. I removed count since I made values use the alias so I figured might as well subsume it.

return gui::button::TYPE_IMAGE;
} else if(type == "radiobox") {
return gui::button::TYPE_RADIO;
} else if(type == "turbo") {
Copy link
Member

Choose a reason for hiding this comment

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

…what on earth is a "turbo"?

(Not an issue, I just don't understand.)

Copy link
Member Author

Choose a reason for hiding this comment

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

No idea!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AI Issues with the AI engine, including micro AIs. Lua API Issues with the Lua engine and API. Terrain Issues that involve terrain definitions or their implementation in the engine. UI User interface issues, including both back-end and front-end issues. Units Issues that involve unit definitions or their implementation in the engine.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants