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.

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

[EXPORTER] fix clang-tidy warnings in UrlParser #3146

Merged
merged 5 commits into from
Nov 19, 2024
Merged

[EXPORTER] fix clang-tidy warnings in UrlParser #3146

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
478139d
[EXPORTER] fix clang-tidy warnings in UrlParser
sjinks Nov 15, 2024
43cfd7c
fix: apply suggestions from code review comments
sjinks Nov 19, 2024
c8749e3
refactor: optimize `UrlDecoder::Decode()`
sjinks Nov 19, 2024
e59a4b1
fix: address code review comments
sjinks Nov 19, 2024
40da6ba
Merge branch 'main' into refactor-urlparser
marcalff Nov 19, 2024
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
43 changes: 22 additions & 21 deletions ext/include/opentelemetry/ext/http/common/url_parser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <cstdint>
#include <cstdlib>
#include <string>
#include <utility>

#include "opentelemetry/version.h"

Expand Down Expand Up @@ -34,7 +35,7 @@ class UrlParser
std::string query_;
bool success_;

UrlParser(std::string url) : url_(url), success_(true)
UrlParser(std::string url) : url_(std::move(url)), success_(true)
marcalff marked this conversation as resolved.
Show resolved Hide resolved
{
if (url_.length() == 0)
{
Expand All @@ -50,15 +51,15 @@ class UrlParser
}
else
{
scheme_ = std::string(url_.begin() + cpos, url_.begin() + pos);
scheme_ = url_.substr(cpos, pos - cpos);
cpos = pos + 3;
}

// credentials
size_t pos1 = url_.find_first_of("@", cpos);
size_t pos2 = url_.find_first_of("/", cpos);
size_t pos1 = url_.find_first_of('@', cpos);
marcalff marked this conversation as resolved.
Show resolved Hide resolved
if (pos1 != std::string::npos)
{
size_t pos2 = url_.find_first_of('/', cpos);
// TODO - handle credentials
if (pos2 == std::string::npos || pos1 < pos2)
{
Expand All @@ -72,15 +73,19 @@ class UrlParser
{
// port missing. Used default 80 / 443
if (scheme_ == "http")
{
port_ = 80;
if (scheme_ == "https")
}
else if (scheme_ == "https")
marcalff marked this conversation as resolved.
Show resolved Hide resolved
{
port_ = 443;
}
}
else
{
// port present
is_port = true;
host_ = std::string(url_.begin() + cpos, url_.begin() + pos);
host_ = url_.substr(cpos, pos - cpos);
cpos = pos + 1;
}
pos = url_.find_first_of("/?", cpos);
Expand All @@ -89,27 +94,23 @@ class UrlParser
path_ = std::string("/"); // use default path
if (is_port)
{
std::string port_str(
url_.begin() + static_cast<std::string::difference_type>(cpos),
url_.begin() + static_cast<std::string::difference_type>(url_.length()));

port_ = GetPort(port_str);
auto port_str = url_.substr(cpos, url_.length());
port_ = GetPort(port_str);
}
else
{
host_ = std::string(url_.begin() + cpos, url_.begin() + url_.length());
host_ = url_.substr(cpos, url_.length());
}
return;
}
if (is_port)
{
std::string port_str(url_.begin() + static_cast<std::string::difference_type>(cpos),
url_.begin() + static_cast<std::string::difference_type>(pos));
port_ = GetPort(port_str);
auto port_str = url_.substr(cpos, pos - cpos);
port_ = GetPort(port_str);
}
else
{
host_ = std::string(url_.begin() + cpos, url_.begin() + pos);
host_ = url_.substr(cpos, pos - cpos);
}
cpos = pos;

Expand All @@ -118,21 +119,21 @@ class UrlParser
pos = url_.find('?', cpos);
if (pos == std::string::npos)
{
path_ = std::string(url_.begin() + cpos, url_.begin() + url_.length());
path_ = url_.substr(cpos, url_.length());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here and below: strictly speaking, it should be (cpos, url_.length - cpos) but because we want all remaining characters, we can save a few CPU cycles by not subtracting cpos.

Copy link
Member

Choose a reason for hiding this comment

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

Even simpler, when we want all the remaining substring:

path_ = url_.substr(cpos);

https://en.cppreference.com/w/cpp/string/basic_string/substr

(e.g. if count == npos), the returned substring is [pos, size())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the code.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, much cleaner. Waiting for CI to complete, then merging.

query_ = "";
}
else
{
path_ = std::string(url_.begin() + cpos, url_.begin() + pos);
path_ = url_.substr(cpos, pos - cpos);
cpos = pos + 1;
query_ = std::string(url_.begin() + cpos, url_.begin() + url_.length());
query_ = url_.substr(cpos, url_.length());
}
return;
}
path_ = std::string("/");
if (url_[cpos] == '?')
{
query_ = std::string(url_.begin() + cpos, url_.begin() + url_.length());
query_ = url_.substr(cpos, url_.length());
}
}

Expand Down Expand Up @@ -209,7 +210,7 @@ class UrlDecoder
hex[1] = encoded[++pos];

char *endptr;
long value = strtol(hex, &endptr, 16);
int value = static_cast<int>(std::strtol(hex, &endptr, 16));
marcalff marked this conversation as resolved.
Show resolved Hide resolved

// Invalid input: no valid hex characters after '%'
if (endptr != &hex[2])
Expand Down