Skip to content

Commit

Permalink
[EXPORTER] fix clang-tidy warnings in UrlParser (#3146)
Browse files Browse the repository at this point in the history
  • Loading branch information
sjinks authored Nov 19, 2024
1 parent 8e7a192 commit 4d9cc28
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 39 deletions.
79 changes: 41 additions & 38 deletions ext/include/opentelemetry/ext/http/common/url_parser.h
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)
{
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);
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")
{
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);
port_ = GetPort(port_str);
}
else
{
host_ = std::string(url_.begin() + cpos, url_.begin() + url_.length());
host_ = url_.substr(cpos);
}
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,20 @@ class UrlParser
pos = url_.find('?', cpos);
if (pos == std::string::npos)
{
path_ = std::string(url_.begin() + cpos, url_.begin() + url_.length());
query_ = "";
path_ = url_.substr(cpos);
}
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);
}
return;
}
path_ = std::string("/");
if (url_[cpos] == '?')
{
query_ = std::string(url_.begin() + cpos, url_.begin() + url_.length());
query_ = url_.substr(cpos);
}
}

Expand Down Expand Up @@ -193,36 +193,39 @@ class UrlDecoder
std::string result;
result.reserve(encoded.size());

auto hex_to_int = [](int ch) -> int {
if (ch >= '0' && ch <= '9')
return ch - '0';
if (ch >= 'a' && ch <= 'f')
return ch - 'a' + 10;
if (ch >= 'A' && ch <= 'F')
return ch - 'A' + 10;
return -1;
};

for (size_t pos = 0; pos < encoded.size(); pos++)
{
if (encoded[pos] == '%')
auto c = encoded[pos];
if (c == '%')
{

// Invalid input: less than two characters left after '%'
if (encoded.size() < pos + 3)
if (pos + 2 >= encoded.size())
{
return encoded;
}

char hex[3] = {0};
hex[0] = encoded[++pos];
hex[1] = encoded[++pos];

char *endptr;
long value = strtol(hex, &endptr, 16);
int hi = hex_to_int(encoded[pos + 1]);
int lo = hex_to_int(encoded[pos + 2]);

// Invalid input: no valid hex characters after '%'
if (endptr != &hex[2])
if (hi == -1 || lo == -1)
{
return encoded;
}

result.push_back(static_cast<char>(value));
}
else
{
result.push_back(encoded[pos]);
c = static_cast<char>((hi << 4) | lo);
pos += 2;
}

result.push_back(c);
}

return result;
Expand Down
3 changes: 2 additions & 1 deletion ext/test/http/url_parser_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,8 @@ TEST(UrlDecoderTests, BasicTests)
{"%2x", "%2x"},
{"%20", " "},
{"text%2", "text%2"},
};
{"%20test%zztest", "%20test%zztest"},
{"%20test%2", "%20test%2"}};

for (auto &testsample : testdata)
{
Expand Down

1 comment on commit 4d9cc28

@github-actions
Copy link

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'OpenTelemetry-cpp api Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: 4d9cc28 Previous: 8e7a192 Ratio
BM_SpinLockThrashing/2/process_time/real_time 0.47619806395636666 ms/iter 0.20479489039707852 ms/iter 2.33

This comment was automatically generated by workflow using github-action-benchmark.

Please sign in to comment.