Skip to content

Use StringView and Span in Player #2293

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.

Sign up for GitHub

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

Merged
Ghabry merged 19 commits into from
Aug 25, 2020
Merged

Use StringView and Span in Player #2293

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
797b8c2
Add StringView to Player
mateofio Aug 16, 2020
d8f4c1d
Add Span from liblcf
mateofio Aug 19, 2020
28a1682
Add ReplacePlaceholders benchmark
mateofio Aug 21, 2020
7459617
Utils replacePlaceHolders - use StringView
mateofio Aug 16, 2020
cacd559
Add Utils MakeArray and MakeVector
mateofio Aug 21, 2020
c9f8ab5
ReplacePlaceholders - optimize using span
mateofio Aug 21, 2020
713233f
Use StringView starts_with and ends_with
mateofio Aug 21, 2020
7c807d4
Upper and Lower Case use StringView
mateofio Aug 16, 2020
b015bcc
Decode functions - use StringView
mateofio Aug 16, 2020
82b8330
Utils Tokenize and ForEachLine: use StringView
mateofio Aug 16, 2020
b4561af
Font::GetSize() - use StringView
mateofio Aug 16, 2020
5322ac0
WordWrap: use StringView
mateofio Aug 16, 2020
2dd4993
Text::Draw functions - use StringView
mateofio Aug 16, 2020
0144971
Bitmap::TextDraw - use StringView
mateofio Aug 16, 2020
7624fa5
Optimize Window_Name
mateofio Aug 16, 2020
0c74513
AsyncHandler - use StringView
mateofio Aug 16, 2020
a22859e
Cache: Support StringView
mateofio Aug 16, 2020
e635250
Window_BattleMessage - StringView
mateofio Aug 16, 2020
4097688
Cache: use ToString
mateofio Aug 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@ add_library(${PROJECT_NAME}
src/shake.h
src/shinonome_gothic.h
src/shinonome_mincho.h
src/span.h
src/sprite_airshipshadow.cpp
src/sprite_airshipshadow.h
src/sprite_battler.cpp
Expand All @@ -276,6 +277,7 @@ add_library(${PROJECT_NAME}
src/state.cpp
src/state.h
src/std_clock.h
src/string_view.h
src/system.h
src/teleport_target.h
src/text.cpp
Expand Down
2 changes: 2 additions & 0 deletions Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,7 @@ libeasyrpg_player_a_SOURCES = \
src/sdl_ui.h \
src/shinonome_gothic.h \
src/shinonome_mincho.h \
src/span.h \
src/sprite.cpp \
src/sprite.h \
src/sprite_airshipshadow.h \
Expand All @@ -275,6 +276,7 @@ libeasyrpg_player_a_SOURCES = \
src/state.cpp \
src/state.h \
src/std_clock.h \
src/string_view.h \
src/system.h \
src/teleport_target.h \
src/text.cpp \
Expand Down
19 changes: 19 additions & 0 deletions bench/utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#include <benchmark/benchmark.h>
#include <font.h>
#include <rect.h>
#include <bitmap.h>
#include <text.h>
#include <pixel_format.h>
#include <cache.h>

static void BM_ReplacePlaceholders(benchmark::State& state) {
for (auto _: state) {
Utils::ReplacePlaceholders("One night is %V %U",
Utils::MakeArray('V', 'U'),
Utils::MakeSvArray("Rest", "Do not Rest"));
}
}

BENCHMARK(BM_ReplacePlaceholders);

BENCHMARK_MAIN();
6 changes: 3 additions & 3 deletions src/async_handler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ void AsyncHandler::CreateRequestMapping(const std::string& file) {
#endif
}

FileRequestAsync* AsyncHandler::RequestFile(const std::string& folder_name, const std::string& file_name) {
FileRequestAsync* AsyncHandler::RequestFile(StringView folder_name, StringView file_name) {
auto path = FileFinder::MakePath(folder_name, file_name);

auto* request = GetRequest(path);
Expand All @@ -105,10 +105,10 @@ FileRequestAsync* AsyncHandler::RequestFile(const std::string& folder_name, cons

//Output::Debug("Waiting for {}", path);

return RegisterRequest(std::move(path), folder_name, file_name);
return RegisterRequest(std::move(path), std::string(folder_name), std::string(file_name));
}

FileRequestAsync* AsyncHandler::RequestFile(const std::string& file_name) {
FileRequestAsync* AsyncHandler::RequestFile(StringView file_name) {
return RequestFile(".", file_name);
}

Expand Down
5 changes: 3 additions & 2 deletions src/async_handler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include <memory>
#include <string>
#include <vector>
#include "string_view.h"

class FileRequestAsync;
struct FileRequestResult;
Expand All @@ -48,7 +49,7 @@ namespace AsyncHandler {
* @param file_name Name of the requested file requested.
* @return The async request.
*/
FileRequestAsync* RequestFile(const std::string& folder_name, const std::string& file_name);
FileRequestAsync* RequestFile(StringView folder_name, StringView file_name);
Copy link
Member

@Ghabry Ghabry Aug 24, 2020

Choose a reason for hiding this comment

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

Question: So, a string view is always used when the target function does not take ownership on the string?

Copy link
Contributor Author

@mateofio mateofio Aug 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I wish I could tell you yes, replace all const std::string& with StringView. Unfortunately this is C++ so there is always one big caveat.

StringView is not null terminated, so if at the end of your call stack you end up calling a C library which requires null terminated const char*, you'll end up having to make a copy so you can get a new string with a null byte at the end.

Aside from that caveat, yes use StringView where-ever you need a non-owning reference. Not only is it a generic adapter but the calling convention used is faster (see below).

Span does not have this caveat, fortunately so you can use it anywhere you'd pass a vector by reference.

Performance Note

const string& if you think about it in C terms, is essentially a double pointer const char**. The reference is a pointer to the string object, which contains another pointer to the bytes.

When you pass a StringView you're passing a const char*, removing one level of indirection. In some cases this can help the optimizer with it's aliasing analysis to produce faster code.

The same rule applies to vector and Span.


/**
* Creates a request to a file.
Expand All @@ -60,7 +61,7 @@ namespace AsyncHandler {
* @param file_name Name of the requested file requested.
* @return The async request.
*/
FileRequestAsync* RequestFile(const std::string& file_name);
FileRequestAsync* RequestFile(StringView file_name);

/**
* Checks if any file with important-flag hasn't finished downloading yet.
Expand Down
8 changes: 4 additions & 4 deletions src/bitmap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ void Bitmap::HueChangeBlit(int x, int y, Bitmap const& src, Rect const& src_rect
Blit(dst_rect.x, dst_rect.y, bmp, bmp.GetRect(), Opacity::Opaque());
}

void Bitmap::TextDraw(Rect const& rect, int color, std::string const& text, Text::Alignment align) {
void Bitmap::TextDraw(Rect const& rect, int color, StringView text, Text::Alignment align) {
FontRef font = Font::Default();
Rect text_rect = font->GetSize(text);
int dx = text_rect.width - rect.width;
Expand All @@ -328,13 +328,13 @@ void Bitmap::TextDraw(Rect const& rect, int color, std::string const& text, Text
}
}

void Bitmap::TextDraw(int x, int y, int color, std::string const& text, Text::Alignment align) {
void Bitmap::TextDraw(int x, int y, int color, StringView text, Text::Alignment align) {
auto font = Font::Default();
auto system = Cache::SystemOrBlack();
Text::Draw(*this, x, y, *font, *system, color, text, align);
}

void Bitmap::TextDraw(Rect const& rect, Color color, std::string const& text, Text::Alignment align) {
void Bitmap::TextDraw(Rect const& rect, Color color, StringView text, Text::Alignment align) {
FontRef font = Font::Default();
Rect text_rect = font->GetSize(text);
int dx = text_rect.width - rect.width;
Expand All @@ -353,7 +353,7 @@ void Bitmap::TextDraw(Rect const& rect, Color color, std::string const& text, Te
}
}

void Bitmap::TextDraw(int x, int y, Color color, std::string const& text) {
void Bitmap::TextDraw(int x, int y, Color color, StringView text) {
auto font = Font::Default();
Text::Draw(*this, x, y, *font, color, text);
}
Expand Down
9 changes: 5 additions & 4 deletions src/bitmap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
#include "pixman_image_ptr.h"
#include "opacity.h"
#include "filesystem_stream.h"
#include "string_view.h"

struct Transform;

Expand Down Expand Up @@ -207,7 +208,7 @@ class Bitmap {
* @param text text to draw.
* @param align text alignment.
*/
void TextDraw(int x, int y, int color, std::string const& text, Text::Alignment align = Text::AlignLeft);
void TextDraw(int x, int y, int color, StringView text, Text::Alignment align = Text::AlignLeft);

/**
* Draws text to bitmap using the Font::Default() font.
Expand All @@ -217,7 +218,7 @@ class Bitmap {
* @param text text to draw.
* @param align text alignment inside bounding rectangle.
*/
void TextDraw(Rect const& rect, int color, std::string const& text, Text::Alignment align = Text::AlignLeft);
void TextDraw(Rect const& rect, int color, StringView text, Text::Alignment align = Text::AlignLeft);

/**
* Draws text to bitmap using the Font::Default() font.
Expand All @@ -227,7 +228,7 @@ class Bitmap {
* @param color text color.
* @param text text to draw.
*/
void TextDraw(int x, int y, Color color, std::string const& text);
void TextDraw(int x, int y, Color color, StringView text);

/**
* Draws text to bitmap using the Font::Default() font.
Expand All @@ -237,7 +238,7 @@ class Bitmap {
* @param text text to draw.
* @param align text alignment inside bounding rectangle.
*/
void TextDraw(Rect const& rect, Color color, std::string const& text, Text::Alignment align = Text::AlignLeft);
void TextDraw(Rect const& rect, Color color, StringView, Text::Alignment align = Text::AlignLeft);

/**
* Blits source bitmap to this one.
Expand Down
65 changes: 33 additions & 32 deletions src/cache.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,29 +39,29 @@
using namespace std::chrono_literals;

namespace {
std::string MakeHashKey(const std::string& folder_name, const std::string& filename, bool transparent) {
return folder_name + ":" + filename + ":" + (transparent ? "T" : " ");
std::string MakeHashKey(StringView folder_name, StringView filename, bool transparent) {
return std::string(folder_name) + ":" + std::string(filename) + ":" + (transparent ? "T" : " ");
Copy link
Member

@Ghabry Ghabry Aug 24, 2020

Choose a reason for hiding this comment

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

Question: Couldn't you also use ToString() here instead of std::string(). Does it make a difference?

Copy link
Contributor Author

@mateofio mateofio Aug 24, 2020

Choose a reason for hiding this comment

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

Ahh this was from the other PR before I added ToString. Will fix.

They do the same thing in this case, but ToString is easier to grep for.

}

std::string MakeTileHashKey(const std::string& chipset_name, int id) {
std::string MakeTileHashKey(StringView chipset_name, int id) {
std::string key;
key.reserve(chipset_name.size() + sizeof(int) + 2);
key.append(reinterpret_cast<char*>(&id), sizeof(id));
key.append(1, ':');
key.append(chipset_name);
key.append(chipset_name.begin(), chipset_name.end());

return key;
}

int IdFromTileHash(const std::string& key) {
int IdFromTileHash(StringView key) {
int id = 0;
if (key.size() > sizeof(id)) {
std::memcpy(&id, key.data(), sizeof(id));
}
return id;
}

const char* NameFromTileHash(const std::string& key) {
const char* NameFromTileHash(StringView key) {
int offset = sizeof(int) + 1;
if (static_cast<int>(key.size()) < offset) {
return "";
Expand Down Expand Up @@ -132,14 +132,15 @@ namespace {
return (cache[key] = {bmp, Game_Clock::GetFrameTime()}).bitmap;
}

BitmapRef LoadBitmap(const std::string& folder_name, const std::string& filename,
BitmapRef LoadBitmap(StringView folder_name, StringView filename,
bool transparent, const uint32_t flags) {
const auto key = MakeHashKey(folder_name, filename, transparent);

auto it = cache.find(key);

if (it == cache.end()) {
const std::string path = FileFinder::FindImage(folder_name, filename);
// FIXME: STRING_VIEW string copies here
const std::string path = FileFinder::FindImage(std::string(folder_name), std::string(filename));

BitmapRef bmp = BitmapRef();

Expand Down Expand Up @@ -249,7 +250,7 @@ namespace {
}

template<Material::Type T>
BitmapRef LoadDummyBitmap(const std::string& folder_name, const std::string& filename, bool transparent) {
BitmapRef LoadDummyBitmap(StringView folder_name, StringView filename, bool transparent) {
static_assert(Material::REND < T && T < Material::END, "Invalid material.");

const Spec& s = spec[T];
Expand All @@ -271,7 +272,7 @@ namespace {
}

template<Material::Type T>
BitmapRef LoadBitmap(const std::string& f, bool transparent) {
BitmapRef LoadBitmap(StringView f, bool transparent) {
static_assert(Material::REND < T && T < Material::END, "Invalid material.");

const Spec& s = spec[T];
Expand Down Expand Up @@ -320,7 +321,7 @@ namespace {
}

template<Material::Type T>
BitmapRef LoadBitmap(const std::string& f) {
BitmapRef LoadBitmap(StringView f) {
static_assert(Material::REND < T && T < Material::END, "Invalid material.");

const Spec& s = spec[T];
Expand All @@ -331,67 +332,67 @@ namespace {

std::vector<uint8_t> Cache::exfont_custom;

BitmapRef Cache::Backdrop(const std::string& file) {
BitmapRef Cache::Backdrop(StringView file) {
return LoadBitmap<Material::Backdrop>(file);
}

BitmapRef Cache::Battle(const std::string& file) {
BitmapRef Cache::Battle(StringView file) {
return LoadBitmap<Material::Battle>(file);
}

BitmapRef Cache::Battle2(const std::string& file) {
BitmapRef Cache::Battle2(StringView file) {
return LoadBitmap<Material::Battle2>(file);
}

BitmapRef Cache::Battlecharset(const std::string& file) {
BitmapRef Cache::Battlecharset(StringView file) {
return LoadBitmap<Material::Battlecharset>(file);
}

BitmapRef Cache::Battleweapon(const std::string& file) {
BitmapRef Cache::Battleweapon(StringView file) {
return LoadBitmap<Material::Battleweapon>(file);
}

BitmapRef Cache::Charset(const std::string& file) {
BitmapRef Cache::Charset(StringView file) {
return LoadBitmap<Material::Charset>(file);
}

BitmapRef Cache::Chipset(const std::string& file) {
BitmapRef Cache::Chipset(StringView file) {
return LoadBitmap<Material::Chipset>(file);
}

BitmapRef Cache::Faceset(const std::string& file) {
BitmapRef Cache::Faceset(StringView file) {
return LoadBitmap<Material::Faceset>(file);
}

BitmapRef Cache::Frame(const std::string& file, bool transparent) {
BitmapRef Cache::Frame(StringView file, bool transparent) {
return LoadBitmap<Material::Frame>(file, transparent);
}

BitmapRef Cache::Gameover(const std::string& file) {
BitmapRef Cache::Gameover(StringView file) {
return LoadBitmap<Material::Gameover>(file);
}

BitmapRef Cache::Monster(const std::string& file) {
BitmapRef Cache::Monster(StringView file) {
return LoadBitmap<Material::Monster>(file);
}

BitmapRef Cache::Panorama(const std::string& file) {
BitmapRef Cache::Panorama(StringView file) {
return LoadBitmap<Material::Panorama>(file);
}

BitmapRef Cache::Picture(const std::string& file, bool transparent) {
BitmapRef Cache::Picture(StringView file, bool transparent) {
return LoadBitmap<Material::Picture>(file, transparent);
}

BitmapRef Cache::System2(const std::string& file) {
BitmapRef Cache::System2(StringView file) {
return LoadBitmap<Material::System2>(file);
}

BitmapRef Cache::Title(const std::string& file) {
BitmapRef Cache::Title(StringView file) {
return LoadBitmap<Material::Title>(file);
}

BitmapRef Cache::System(const std::string& file) {
BitmapRef Cache::System(StringView file) {
return LoadBitmap<Material::System>(file);
}

Expand Down Expand Up @@ -419,7 +420,7 @@ BitmapRef Cache::Exfont() {
}
}

BitmapRef Cache::Tile(const std::string& filename, int tile_id) {
BitmapRef Cache::Tile(StringView filename, int tile_id) {
const auto key = MakeTileHashKey(filename, tile_id);
auto it = cache_tiles.find(key);

Expand Down Expand Up @@ -519,12 +520,12 @@ void Cache::Clear() {
cache_tiles.clear();
}

void Cache::SetSystemName(std::string const& filename) {
system_name = filename;
void Cache::SetSystemName(std::string filename) {
system_name = std::move(filename);
}

void Cache::SetSystem2Name(std::string const& filename) {
system2_name = filename;
void Cache::SetSystem2Name(std::string filename) {
system2_name = std::move(filename);
}

BitmapRef Cache::System() {
Expand Down
Loading