-
Notifications
You must be signed in to change notification settings - Fork 15
code review suggestions: #69
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
code review suggestions: #69
Conversation
- `easy_setopt_timeout` -> `easy_setopt_timeout_ms`
- remove `HTTPClient::Deadline` alias
- pass `Clock` into `CurlImpl`
- `finalize_config(const TracerConfig&, const Clock&)`
- `FinalizedTracerConfig::clock`
- `finalize_config(const DatadogAgentConfig&, const Clock&)`
- `default_http_client(const shared_ptr<Logger>&, const Clock&)`
- `FinalizedDatadogAgentConfig::clock`
- `DatadogAgent::DatadogAgent`
- more specific `Error::CURL_DEADLINE_EXCEEDED_BEFORE_REQUEST_START`,
and a little context in the error message
- `dummy_timeout` -> `dummy_deadline`
- `TEST_CASE` for `Error::CURL_DEADLINE_EXCEEDED_BEFORE_REQUEST_START`
| std::string message; | ||
| message += | ||
| "Request deadline exceeded before request was even added to " | ||
| "libcurl " | ||
| "event loop. Deadline was "; | ||
| message += std::to_string( | ||
| -std::chrono::duration_cast<std::chrono::nanoseconds>(timeout) | ||
| .count()); | ||
| message += " nanoseconds ago."; |
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.
evangelist hat May I suggest libfmt? It is header-only 😀
The fmt version would like look:
std::string message = fmt::format("Request deadline exceeded before request was even added to libcurl event loop. Deadline was {} nanoseconds ago.", std::chrono::duration_cast<std::chrono::nanoseconds>(timeout));Much much easier to read and less error-prone right?
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.
It's not header-only:
david@ein:~/src/fmt$ cat scratch.cpp
#include <fmt/core.h>
#include <fmt/chrono.h>
#include <chrono>
int main() {
const auto delay = std::chrono::microseconds(4);
fmt::print("The delay was {}.", delay);
}
david@ein:~/src/fmt$ g++ -I include scratch.cpp
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `main':
scratch.cpp:(.text+0x8e): undefined reference to `fmt::v10::vprint(fmt::v10::basic_string_view<char>, fmt::v10::basic_format_args<fmt::v10::basic_format_context<fmt::v10::appender, char> >)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `fmt::v10::detail::error_handler::on_error(char const*)':
scratch.cpp:(.text._ZN3fmt3v106detail13error_handler8on_errorEPKc[_ZN3fmt3v106detail13error_handler8on_errorEPKc]+0xe): undefined reference to `fmt::v10::detail::throw_format_error(char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `std::make_unsigned<int>::type fmt::v10::detail::to_unsigned<int>(int)':
scratch.cpp:(.text._ZN3fmt3v106detail11to_unsignedIiEENSt13make_unsignedIT_E4typeES4_[_ZN3fmt3v106detail11to_unsignedIiEENSt13make_unsignedIT_E4typeES4_]+0x23): undefined reference to `fmt::v10::detail::assert_fail(char const*, int, char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `fmt::v10::detail::needs_escape(unsigned int)':
scratch.cpp:(.text._ZN3fmt3v106detail12needs_escapeEj[_ZN3fmt3v106detail12needs_escapeEj]+0x2d): undefined reference to `fmt::v10::detail::is_printable(unsigned int)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `fmt::v10::detail::get_locale::get_locale(bool, fmt::v10::detail::locale_ref)':
scratch.cpp:(.text._ZN3fmt3v106detail10get_localeC2EbNS1_10locale_refE[_ZN3fmt3v106detail10get_localeC5EbNS1_10locale_refE]+0x5d): undefined reference to `std::locale fmt::v10::detail::locale_ref::get<std::locale>() const'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `std::make_unsigned<long>::type fmt::v10::detail::to_unsigned<long>(long)':
scratch.cpp:(.text._ZN3fmt3v106detail11to_unsignedIlEENSt13make_unsignedIT_E4typeES4_[_ZN3fmt3v106detail11to_unsignedIlEENSt13make_unsignedIT_E4typeES4_]+0x25): undefined reference to `fmt::v10::detail::assert_fail(char const*, int, char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `fmt::v10::detail::format_decimal_result<char*> fmt::v10::detail::format_decimal<char, unsigned int>(char*, unsigned int, int)':
scratch.cpp:(.text._ZN3fmt3v106detail14format_decimalIcjEENS1_21format_decimal_resultIPT_EES5_T0_i[_ZN3fmt3v106detail14format_decimalIcjEENS1_21format_decimal_resultIPT_EES5_T0_i]+0x43): undefined reference to `fmt::v10::detail::assert_fail(char const*, int, char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `fmt::v10::detail::format_decimal_result<char*> fmt::v10::detail::format_decimal<char, unsigned long>(char*, unsigned long, int)':
scratch.cpp:(.text._ZN3fmt3v106detail14format_decimalIcmEENS1_21format_decimal_resultIPT_EES5_T0_i[_ZN3fmt3v106detail14format_decimalIcmEENS1_21format_decimal_resultIPT_EES5_T0_i]+0x46): undefined reference to `fmt::v10::detail::assert_fail(char const*, int, char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `char const* fmt::v10::detail::parse_align<char>(char const*, char const*, fmt::v10::format_specs<char>&)':
scratch.cpp:(.text._ZN3fmt3v106detail11parse_alignIcEEPKT_S5_S5_RNS0_12format_specsIS3_EE[_ZN3fmt3v106detail11parse_alignIcEEPKT_S5_S5_RNS0_12format_specsIS3_EE]+0x4c): undefined reference to `fmt::v10::detail::assert_fail(char const*, int, char const*)'
/usr/bin/ld: scratch.cpp:(.text._ZN3fmt3v106detail11parse_alignIcEEPKT_S5_S5_RNS0_12format_specsIS3_EE[_ZN3fmt3v106detail11parse_alignIcEEPKT_S5_S5_RNS0_12format_specsIS3_EE]+0x100): undefined reference to `fmt::v10::detail::throw_format_error(char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `char const* fmt::v10::detail::parse_dynamic_spec<char>(char const*, char const*, int&, fmt::v10::detail::arg_ref<char>&, fmt::v10::basic_format_parse_context<char>&)':
scratch.cpp:(.text._ZN3fmt3v106detail18parse_dynamic_specIcEEPKT_S5_S5_RiRNS1_7arg_refIS3_EERNS0_26basic_format_parse_contextIS3_EE[_ZN3fmt3v106detail18parse_dynamic_specIcEEPKT_S5_S5_RiRNS1_7arg_refIS3_EERNS0_26basic_format_parse_contextIS3_EE]+0x71): undefined reference to `fmt::v10::detail::assert_fail(char const*, int, char const*)'
/usr/bin/ld: scratch.cpp:(.text._ZN3fmt3v106detail18parse_dynamic_specIcEEPKT_S5_S5_RiRNS1_7arg_refIS3_EERNS0_26basic_format_parse_contextIS3_EE[_ZN3fmt3v106detail18parse_dynamic_specIcEEPKT_S5_S5_RiRNS1_7arg_refIS3_EERNS0_26basic_format_parse_contextIS3_EE]+0x9a): undefined reference to `fmt::v10::detail::throw_format_error(char const*)'
/usr/bin/ld: scratch.cpp:(.text._ZN3fmt3v106detail18parse_dynamic_specIcEEPKT_S5_S5_RiRNS1_7arg_refIS3_EERNS0_26basic_format_parse_contextIS3_EE[_ZN3fmt3v106detail18parse_dynamic_specIcEEPKT_S5_S5_RiRNS1_7arg_refIS3_EERNS0_26basic_format_parse_contextIS3_EE]+0x111): undefined reference to `fmt::v10::detail::throw_format_error(char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `char const* fmt::v10::detail::parse_precision<char>(char const*, char const*, int&, fmt::v10::detail::arg_ref<char>&, fmt::v10::basic_format_parse_context<char>&)':
scratch.cpp:(.text._ZN3fmt3v106detail15parse_precisionIcEEPKT_S5_S5_RiRNS1_7arg_refIS3_EERNS0_26basic_format_parse_contextIS3_EE[_ZN3fmt3v106detail15parse_precisionIcEEPKT_S5_S5_RiRNS1_7arg_refIS3_EERNS0_26basic_format_parse_contextIS3_EE]+0x2c): undefined reference to `fmt::v10::detail::throw_format_error(char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `fmt::v10::detail::fill_t<char>::operator=(fmt::v10::basic_string_view<char>)':
scratch.cpp:(.text._ZN3fmt3v106detail6fill_tIcEaSENS0_17basic_string_viewIcEE[_ZN3fmt3v106detail6fill_tIcEaSENS0_17basic_string_viewIcEE]+0x45): undefined reference to `fmt::v10::detail::assert_fail(char const*, int, char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `int fmt::v10::detail::parse_nonnegative_int<char>(char const*&, char const*, int)':
scratch.cpp:(.text._ZN3fmt3v106detail21parse_nonnegative_intIcEEiRPKT_S5_i[_ZN3fmt3v106detail21parse_nonnegative_intIcEEiRPKT_S5_i]+0x7a): undefined reference to `fmt::v10::detail::assert_fail(char const*, int, char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `int fmt::v10::detail::to_nonnegative_int<long, int, 0>(long, int)':
scratch.cpp:(.text._ZN3fmt3v106detail18to_nonnegative_intIliLi0EEET0_T_S3_[_ZN3fmt3v106detail18to_nonnegative_intIliLi0EEET0_T_S3_]+0x55): undefined reference to `fmt::v10::detail::assert_fail(char const*, int, char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `char const* fmt::v10::detail::do_parse_arg_id<char, fmt::v10::detail::dynamic_spec_id_handler<char>&>(char const*, char const*, fmt::v10::detail::dynamic_spec_id_handler<char>&)':
scratch.cpp:(.text._ZN3fmt3v106detail15do_parse_arg_idIcRNS1_23dynamic_spec_id_handlerIcEEEEPKT_S8_S8_OT0_[_ZN3fmt3v106detail15do_parse_arg_idIcRNS1_23dynamic_spec_id_handlerIcEEEEPKT_S8_S8_OT0_]+0x8f): undefined reference to `fmt::v10::detail::throw_format_error(char const*)'
/usr/bin/ld: scratch.cpp:(.text._ZN3fmt3v106detail15do_parse_arg_idIcRNS1_23dynamic_spec_id_handlerIcEEEEPKT_S8_S8_OT0_[_ZN3fmt3v106detail15do_parse_arg_idIcRNS1_23dynamic_spec_id_handlerIcEEEEPKT_S8_S8_OT0_]+0x11d): undefined reference to `fmt::v10::detail::throw_format_error(char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `fmt::v10::detail::tm_writer<std::back_insert_iterator<fmt::v10::basic_memory_buffer<char, 500ul, std::allocator<char> > >, char, std::chrono::duration<long, std::ratio<1l, 1l> > >::tm_hour() const':
scratch.cpp:(.text._ZNK3fmt3v106detail9tm_writerISt20back_insert_iteratorINS0_19basic_memory_bufferIcLm500ESaIcEEEEcNSt6chrono8durationIlSt5ratioILl1ELl1EEEEE7tm_hourEv[_ZNK3fmt3v106detail9tm_writerISt20back_insert_iteratorINS0_19basic_memory_bufferIcLm500ESaIcEEEEcNSt6chrono8durationIlSt5ratioILl1ELl1EEEEE7tm_hourEv]+0x49): undefined reference to `fmt::v10::detail::assert_fail(char const*, int, char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `fmt::v10::detail::tm_writer<std::back_insert_iterator<fmt::v10::basic_memory_buffer<char, 500ul, std::allocator<char> > >, char, std::chrono::duration<long, std::ratio<1l, 1l> > >::tm_min() const':
scratch.cpp:(.text._ZNK3fmt3v106detail9tm_writerISt20back_insert_iteratorINS0_19basic_memory_bufferIcLm500ESaIcEEEEcNSt6chrono8durationIlSt5ratioILl1ELl1EEEEE6tm_minEv[_ZNK3fmt3v106detail9tm_writerISt20back_insert_iteratorINS0_19basic_memory_bufferIcLm500ESaIcEEEEcNSt6chrono8durationIlSt5ratioILl1ELl1EEEEE6tm_minEv]+0x49): undefined reference to `fmt::v10::detail::assert_fail(char const*, int, char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `std::make_unsigned<long long>::type fmt::v10::detail::to_unsigned<long long>(long long)':
scratch.cpp:(.text._ZN3fmt3v106detail11to_unsignedIxEENSt13make_unsignedIT_E4typeES4_[_ZN3fmt3v106detail11to_unsignedIxEENSt13make_unsignedIT_E4typeES4_]+0x25): undefined reference to `fmt::v10::detail::assert_fail(char const*, int, char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `fmt::v10::detail::tm_writer<std::back_insert_iterator<fmt::v10::basic_memory_buffer<char, 500ul, std::allocator<char> > >, char, std::chrono::duration<long, std::ratio<1l, 1l> > >::tm_sec() const':
scratch.cpp:(.text._ZNK3fmt3v106detail9tm_writerISt20back_insert_iteratorINS0_19basic_memory_bufferIcLm500ESaIcEEEEcNSt6chrono8durationIlSt5ratioILl1ELl1EEEEE6tm_secEv[_ZNK3fmt3v106detail9tm_writerISt20back_insert_iteratorINS0_19basic_memory_bufferIcLm500ESaIcEEEEcNSt6chrono8durationIlSt5ratioILl1ELl1EEEEE6tm_secEv]+0x47): undefined reference to `fmt::v10::detail::assert_fail(char const*, int, char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `fmt::v10::basic_format_parse_context<char>::next_arg_id()':
scratch.cpp:(.text._ZN3fmt3v1026basic_format_parse_contextIcE11next_arg_idEv[_ZN3fmt3v1026basic_format_parse_contextIcE11next_arg_idEv]+0x1e): undefined reference to `fmt::v10::detail::throw_format_error(char const*)'
/usr/bin/ld: /tmp/ccdUMaH7.o: in function `fmt::v10::basic_format_parse_context<char>::check_arg_id(int)':
scratch.cpp:(.text._ZN3fmt3v1026basic_format_parse_contextIcE12check_arg_idEi[_ZN3fmt3v1026basic_format_parse_contextIcE12check_arg_idEi]+0x1e): undefined reference to `fmt::v10::detail::throw_format_error(char const*)'
collect2: error: ld returned 1 exit statusAnd, not that this matters much, it's a lot more code:
david@ein:~/src/fmt$ g++ -I include -E scratch.cpp | wc -l
82663
david@ein:~/src/fmt$ cat nofmt.cpp
#include <chrono>
#include <string>
extern "C" int puts(const char*);
int main() {
const auto delay = std::chrono::microseconds(4);
std::string message;
message += "The delay was ";
message += std::to_string(std::chrono::duration_cast<std::chrono::nanoseconds>(delay).count());
message += " nanoseconds.";
puts(message.c_str());
}
david@ein:~/src/fmt$ g++ -I include -E nofmt.cpp | wc -l
26112std::format is in C++20, so we could use that in the next tracing library. :)
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. Thank you for the new test case. ![]()
- `easy_setopt_timeout` -> `easy_setopt_timeout_ms`
- remove `HTTPClient::Deadline` alias
- pass `Clock` into `CurlImpl`
- `finalize_config(const TracerConfig&, const Clock&)`
- `FinalizedTracerConfig::clock`
- `finalize_config(const DatadogAgentConfig&, const Clock&)`
- `default_http_client(const shared_ptr<Logger>&, const Clock&)`
- `FinalizedDatadogAgentConfig::clock`
- `DatadogAgent::DatadogAgent`
- more specific `Error::CURL_DEADLINE_EXCEEDED_BEFORE_REQUEST_START`,
and a little context in the error message
- `dummy_timeout` -> `dummy_deadline`
- `TEST_CASE` for `Error::CURL_DEADLINE_EXCEEDED_BEFORE_REQUEST_START`
easy_setopt_timeout->easy_setopt_timeout_msHTTPClient::DeadlinealiasClockintoCurlImplfinalize_config(const TracerConfig&, const Clock&)FinalizedTracerConfig::clockfinalize_config(const DatadogAgentConfig&, const Clock&)default_http_client(const shared_ptr<Logger>&, const Clock&)FinalizedDatadogAgentConfig::clockDatadogAgent::DatadogAgentError::CURL_DEADLINE_EXCEEDED_BEFORE_REQUEST_START, and a little context in the error messagedummy_timeout->dummy_deadlineTEST_CASEforError::CURL_DEADLINE_EXCEEDED_BEFORE_REQUEST_START