-
Notifications
You must be signed in to change notification settings - Fork 449
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
[EXPORTER] handling of invalid ports in UrlParser #3142
Conversation
✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.
|
url_.begin() + static_cast<std::string::difference_type>(cpos), | ||
url_.begin() + static_cast<std::string::difference_type>(url_.length())); |
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.
static_cast<>
is because "Narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'difference_type' (aka 'long') is implementation-defined"
See also: https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/narrowing-conversions.html
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3142 +/- ##
==========================================
+ Coverage 87.12% 87.83% +0.72%
==========================================
Files 200 195 -5
Lines 6109 6146 +37
==========================================
+ Hits 5322 5398 +76
+ Misses 787 748 -39
|
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.
LGTM, thanks for the fix.
[EXPORTER] handling of invalid ports in UrlParser (open-telemetry#3142)
Fixes #3138
Changes
This PR adds handling of invalid ports to UrlParser.
Previously, invalid ports caused an exception; this PR updated the code so as not to throw exceptions and set
success_
tofalse
.For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes