-
-
Notifications
You must be signed in to change notification settings - Fork 450
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
dev: Add RecentMessages benchmark #5071
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small nitpicks/questions - overall looks good. The changes to TwitchChannel/TwitchBadges etc are acceptable
I haven't looked at the actual benchmark yet, will do that tomorrow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
@@ -145,16 +145,13 @@ static void BM_EmojiParsing(benchmark::State &state) | |||
|
|||
BENCHMARK(BM_EmojiParsing); | |||
|
|||
template <class... Args> | |||
static void BM_EmojiParsing2(benchmark::State &state, Args &&...args) | |||
static void BM_EmojiParsing2(benchmark::State &state, const QString &input, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: function 'BM_EmojiParsing2' declared 'static', move to anonymous namespace instead [misc-use-anonymous-namespace]
^
return *opt; | ||
} | ||
|
||
class RecentMessages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: class 'RecentMessages' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
class RecentMessages
^
return *opt; | ||
} | ||
|
||
class RecentMessages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: destructor of 'RecentMessages' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
class RecentMessages
^
Additional context
benchmarks/src/RecentMessages.cpp:112: make it public and virtual
class RecentMessages
^
explicit RecentMessages(const QString &name_) | ||
: name(name_) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: pass by value and use std::move [modernize-pass-by-value]
benchmarks/src/RecentMessages.cpp:25:
+ #include <utility>
explicit RecentMessages(const QString &name_) | |
: name(name_) | |
explicit RecentMessages(QString name_) | |
: name(std::move(name_)) |
virtual void run(benchmark::State &state) = 0; | ||
|
||
protected: | ||
QString name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: invalid case style for protected member 'name' [readability-identifier-naming]
benchmarks/src/RecentMessages.cpp:116:
- : name(name_)
- , chan(this->name)
+ : name_(name_)
+ , chan(this->name_)
benchmarks/src/RecentMessages.cpp:120:
- tryReadJsonFile(u":/bench/seventvemotes-%1.json"_s.arg(this->name));
+ tryReadJsonFile(u":/bench/seventvemotes-%1.json"_s.arg(this->name_));
benchmarks/src/RecentMessages.cpp:122:
- tryReadJsonFile(u":/bench/bttvemotes-%1.json"_s.arg(this->name));
+ tryReadJsonFile(u":/bench/bttvemotes-%1.json"_s.arg(this->name_));
benchmarks/src/RecentMessages.cpp:124:
- tryReadJsonFile(u":/bench/ffzemotes-%1.json"_s.arg(this->name));
+ tryReadJsonFile(u":/bench/ffzemotes-%1.json"_s.arg(this->name_));
benchmarks/src/RecentMessages.cpp:140:
- this->name)));
+ this->name_)));
benchmarks/src/RecentMessages.cpp:150:
- readJsonFile(u":/bench/recentmessages-%1.json"_s.arg(this->name));
+ readJsonFile(u":/bench/recentmessages-%1.json"_s.arg(this->name_));
QString name; | |
QString name_; |
QString name; | ||
MockApplication app; | ||
TwitchChannel chan; | ||
QJsonDocument messages; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: invalid case style for protected member 'messages' [readability-identifier-naming]
benchmarks/src/RecentMessages.cpp:149:
- this->messages =
+ this->messages_ =
QJsonDocument messages; | |
QJsonDocument messages_; |
benchmarks/src/RecentMessages.cpp:180:
- this->messages.object());
+ this->messages_.object());
benchmarks/src/RecentMessages.cpp:197:
- this->messages.object());
+ this->messages_.object());
QJsonDocument messages; | ||
}; | ||
|
||
class ParseRecentMessages : public RecentMessages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: destructor of 'ParseRecentMessages' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
class ParseRecentMessages : public RecentMessages
^
Additional context
benchmarks/src/RecentMessages.cpp:167: make it public and virtual
class ParseRecentMessages : public RecentMessages
^
{ | ||
} | ||
|
||
void run(benchmark::State &state) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: annotate this function with 'override' or (rarely) 'final' [cppcoreguidelines-explicit-virtual-functions]
void run(benchmark::State &state) | |
void run(benchmark::State &state) override |
} | ||
}; | ||
|
||
class BuildRecentMessages : public RecentMessages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: destructor of 'BuildRecentMessages' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
class BuildRecentMessages : public RecentMessages
^
Additional context
benchmarks/src/RecentMessages.cpp:186: make it public and virtual
class BuildRecentMessages : public RecentMessages
^
{ | ||
} | ||
|
||
void run(benchmark::State &state) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: annotate this function with 'override' or (rarely) 'final' [cppcoreguidelines-explicit-virtual-functions]
void run(benchmark::State &state) | |
void run(benchmark::State &state) override |
This PR adds a benchmark for parsing and building recent messages. The changes to
TwitchChannel
aren't super nice, but I think they're relatively fine. This also adds the ability to test BTTV, FFZ, and 7TV emote parsing (and recent message parsing & building).