[EXPORTER] fix clang-tidy warnings in UrlParser #3146
Conversation
✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3146 +/- ##
==========================================
+ Coverage 87.12% 87.86% +0.74%
==========================================
Files 200 195 -5
Lines 6109 6151 +42
==========================================
+ Hits 5322 5404 +82
+ Misses 787 747 -40
|
c872a71 to
43cfd7c
Compare
| path_ = std::string(url_.begin() + cpos, url_.begin() + url_.length()); | ||
| path_ = url_.substr(cpos, url_.length()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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())
There was a problem hiding this comment.
Thanks, much cleaner. Waiting for CI to complete, then merging.
|
I also tried to optimize |
|
@marcalff a bit of offtopic: I would like to make tests more readable by using parametrized tests. Among other things, this will show which of the test data failed. Something like this: class UrlDecoderTests : public ::testing::TestWithParam<std::tuple<std::string, std::string>>
{};
INSTANTIATE_TEST_SUITE_P(
SampleValues,
UrlDecoderTests,
testing::Values(std::make_tuple("Authentication=Basic xxx", "Authentication=Basic xxx"),
std::make_tuple("Authentication=Basic%20xxx", "Authentication=Basic xxx"),
std::make_tuple("%C3%B6%C3%A0%C2%A7%C3%96abcd%C3%84",
"\xc3\xb6\xc3\xa0\xc2\xa7\xc3\x96\x61\x62\x63\x64\xc3\x84"),
std::make_tuple("%2x", "%2x"),
std::make_tuple("%20", " "),
std::make_tuple("text%2", "text%2")));
TEST_P(UrlDecoderTests, BasicTests)
{
const auto encoded = std::get<0>(GetParam());
const auto expected = std::get<1>(GetParam());
const auto actual = http_common::UrlDecoder::Decode(encoded);
EXPECT_EQ(actual, expected);
}instead of TEST(UrlDecoderTests, BasicTests)
{
std::map<std::string, std::string> testdata{
{"Authentication=Basic xxx", "Authentication=Bas
8000
ic xxx"},
{"Authentication=Basic%20xxx", "Authentication=Basic xxx"},
{"%C3%B6%C3%A0%C2%A7%C3%96abcd%C3%84",
"\xc3\xb6\xc3\xa0\xc2\xa7\xc3\x96\x61\x62\x63\x64\xc3\x84"},
{"%2x", "%2x"},
{"%20", " "},
{"text%2", "text%2"},
};
for (auto &testsample : testdata)
{
ASSERT_EQ(http_common::UrlDecoder::Decode(testsample.first), testsample.second);
ASSERT_TRUE(http_common::UrlDecoder::Decode(testsample.first) == testsample.second);
}
}The output will look like this (I intentionally made the test fail): See http://google.github.io/googletest/advanced.html#value-parameterized-tests Is this something you would accept as a PR? |
(...)
For a new test case added, I would definitively take it, this is good code. For a PR that refactors test cases alone, say to investigate a hard to reproduce spurious failure to pinpoint which case is failing exactly, I would also take it. The problem here is that we can not have refactoring both of production code (the url parser) and of unit tests for it, at the same time in the same PR. This introduces the risk of adding a bug that goes undetected, even when admittedly the test case refactoring looks clean, so the probability of a bug in the unit test alone hiding a bug in the production code is very low. That being said, I like the refactored test, as it will show how to write parameterized tests in opentelemetry-cpp in general, serving as an example for others, so I think it is important to have it. As long as the unit test is refactored in a different PR (I think this is what you meant), I will (gladly) take it. The test PR can be reviewed and merged immediately. |
The URL parser is used once at program startup, to parse the configuration of a OTLP exporter for example, so performance is hardly a concern, as this is not in any critical path. The refactored code is also much cleaner, so even if this is a bit more than just fixing a clang-tidy warning, it is a good change to have (TLDR: approved). |
Changes
This PR fixes issues found by
clang-tidyin UrlParser.Please provide a brief description of the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes