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

Jump to bottom

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+25 −13
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
38 changes: 25 additions & 13 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)
Comment on lines -37 to +38
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Parameter 'url' is passed by value and only copied once; consider moving it to avoid unnecessary copies

https://clang.llvm.org/extra/clang-tidy/checks/performance/unnecessary-value-param.html

{
if (url_.length() == 0)
{
Expand All @@ -50,15 +51,16 @@ class UrlParser
}
else
{
scheme_ = std::string(url_.begin() + cpos, url_.begin() + pos);
scheme_ = std::string(url_.begin() + static_cast<std::string::difference_type>(cpos),
url_.begin() + static_cast<std::string::difference_type>(pos));
Comment on lines -53 to +55
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one and similar issues:

Narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'difference_type' (aka 'long') is implementation-defined

https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/narrowing-conversions.html

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);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

'find_first_of' called with a string literal consisting of a single character; consider using the more effective overload accepting a character

https://clang.llvm.org/extra/clang-tidy/checks/performance/faster-string-find.html

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 @@ -80,7 +82,8 @@ class UrlParser
{
// port present
is_port = true;
host_ = std::string(url_.begin() + cpos, url_.begin() + pos);
host_ = std::string(url_.begin() + static_cast<std::string::difference_type>(cpos),
url_.begin() + static_cast<std::string::difference_type>(pos));
cpos = pos + 1;
}
pos = url_.find_first_of("/?", cpos);
Expand All @@ -97,7 +100,9 @@ class UrlParser
}
else
{
host_ = std::string(url_.begin() + cpos, url_.begin() + url_.length());
host_ =
std::string(url_.begin() + static_cast<std::string::difference_type>(cpos),
url_.begin() + static_cast<std::string::difference_type>(url_.length()));
}
return;
}
Expand All @@ -109,7 +114,8 @@ class UrlParser
}
else
{
host_ = std::string(url_.begin() + cpos, url_.begin() + pos);
host_ = std::string(url_.begin() + static_cast<std::string::difference_type>(cpos),
url_.begin() + static_cast<std::string::difference_type>(pos));
}
cpos = pos;

Expand All @@ -118,21 +124,27 @@ class UrlParser
pos = url_.find('?', cpos);
if (pos == std::string::npos)
{
path_ = std::string(url_.begin() + cpos, url_.begin() + url_.length());
path_ =
std::string(url_.begin() + static_cast<std::string::difference_type>(cpos),
url_.begin() + static_cast<std::string::difference_type>(url_.length()));
query_ = "";
}
else
{
path_ = std::string(url_.begin() + cpos, url_.begin() + pos);
cpos = pos + 1;
query_ = std::string(url_.begin() + cpos, url_.begin() + url_.length());
path_ = std::string(url_.begin() + static_cast<std::string::difference_type>(cpos),
url_.begin() + static_cast<std::string::difference_type>(pos));
cpos = pos + 1;
query_ =
std::string(url_.begin() + static_cast<std::string::difference_type>(cpos),
url_.begin() + static_cast<std::string::difference_type>(url_.length()));
}
return;
}
path_ = std::string("/");
if (url_[cpos] == '?')
{
query_ = std::string(url_.begin() + cpos, url_.begin() + url_.length());
query_ = std::string(url_.begin() + static_cast<std::string::difference_type>(cpos),
url_.begin() + static_cast<std::string::difference_type>(url_.length()));
}
}

Expand Down Expand Up @@ -209,7 +221,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));
Comment on lines -212 to +224
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The google-runtime-int rule suggests replacing 'long' with 'int64', which seems overkill. Because two hexadecimal characters give a value in the [0; 255] range, downcasting to int is safe.


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