-
-
Notifications
You must be signed in to change notification settings - Fork 442
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
Remove Redundant Parsing of Links #4507
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
@@ -67,7 +67,7 @@ namespace { | |||
|
|||
LinkParser::LinkParser(const QString &unparsedString) | |||
{ | |||
this->match_ = unparsedString; | |||
ParsedLink result; |
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: variable 'result' of type 'chatterino::ParsedLink' can be declared 'const' [misc-const-correctness]
ParsedLink result; | |
ParsedLink const result; |
@@ -92,8 +94,10 @@ LinkParser::LinkParser(const QString &unparsedString) | |||
|
|||
// Host `a.b.c.com` | |||
QStringRef host = l; | |||
ParsedLink::StringView rest; |
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: variable 'rest' is not initialized [cppcoreguidelines-init-variables]
ParsedLink::StringView rest; | |
ParsedLink::StringView rest = 0; |
bool lastWasDot = true; | ||
bool inIpv6 = false; | ||
bool hasMatch = false; |
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: variable 'hasMatch' of type 'bool' can be declared 'const' [misc-const-correctness]
bool hasMatch = false; | |
bool const hasMatch = false; |
@@ -2,19 +2,31 @@ | |||
|
|||
#include <QString> |
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: 'QString' file not found [clang-diagnostic-error]
#include <QString>
^
namespace chatterino { | ||
|
||
struct ParsedLink { |
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: constructor does not initialize these fields: protocol, host, rest, source [cppcoreguidelines-pro-type-member-init]
src/common/LinkParser.hpp:14:
- StringView protocol;
- StringView host;
- StringView rest;
- QString source;
+ StringView protocol{};
+ StringView host{};
+ StringView rest{};
+ QString source{};
Pull request checklist:
CHANGELOG.md
was updated, if applicableDescription
This is a followup to #4436. While refactoring the
LinkParser
, I noticed that when matching/adding links, they're essentially parsed three times - first by the link-parser, then with up to three regexes and lastly by another regex.All the regex-parsing can be eliminated:
ftp
andspotify
regexes aren't needed, because these types of links will never be parsed by theLinkParser
.https?
regex can be removed, because theLinkParser
knows whether the link contains a protocol or not.LinkParser
knows where the host part of the link is (sidenote: the current regex is even wrong, if you have a link such aschatterino.com#FOO
, then it will display aschatterino.com#foo
).I updated the tests to match the new format and added some tests that check the case-insensitivity of the parser. This doesn't change the parsing inside
LinkParser
yet.