Copilot commented on code in PR #556:
URL: https://github.com/apache/iceberg-cpp/pull/556#discussion_r2777454215
##########
src/iceberg/util/string_util.h:
##########
@@ -67,17 +67,22 @@ class ICEBERG_EXPORT StringUtils {
}
template <typename T>
- static Result<T> ParseInt(std::string_view str) {
+ requires std::is_arithmetic_v<T> && (!std::same_as<T, bool>)
+ static Result<T> ParseNumber(std::string_view str) {
T value = 0;
auto [ptr, ec] = std::from_chars(str.data(), str.data() + str.size(),
value);
- if (ec == std::errc::invalid_argument) [[unlikely]] {
- return InvalidArgument("Failed to parse integer from string '{}':
invalid argument",
- str);
- } else if (ec == std::errc::result_out_of_range) [[unlikely]] {
+ if (ec == std::errc()) [[likely]] {
+ return value;
+ }
Review Comment:
Renaming/removing `StringUtils::ParseInt` in a public header is a
source-breaking API change. The project docs call out that public methods
should not be removed without a deprecation period; consider keeping `ParseInt`
as a deprecated wrapper (and possibly `ParseFloat`/`ParseDouble` wrappers if
needed) that forwards to `ParseNumber` to preserve compatibility.
##########
src/iceberg/util/string_util.h:
##########
@@ -67,17 +67,22 @@ class ICEBERG_EXPORT StringUtils {
}
template <typename T>
- static Result<T> ParseInt(std::string_view str) {
+ requires std::is_arithmetic_v<T> && (!std::same_as<T, bool>)
+ static Result<T> ParseNumber(std::string_view str) {
T value = 0;
auto [ptr, ec] = std::from_chars(str.data(), str.data() + str.size(),
value);
- if (ec == std::errc::invalid_argument) [[unlikely]] {
- return InvalidArgument("Failed to parse integer from string '{}':
invalid argument",
- str);
- } else if (ec == std::errc::result_out_of_range) [[unlikely]] {
+ if (ec == std::errc()) [[likely]] {
+ return value;
+ }
+ if (ec == std::errc::invalid_argument) {
+ return InvalidArgument("Failed to parse {} from string '{}': invalid
argument",
+ typeid(T).name(), str);
+ }
+ if (ec == std::errc::result_out_of_range) {
return InvalidArgument("Failed to parse {} from string '{}': value out
of range",
typeid(T).name(), str);
}
Review Comment:
The new error messages use `typeid(T).name()`, which is
implementation-defined/mangled and also changes the previous stable wording
(e.g., "Failed to parse integer from string..."). At least one test matches
that substring, so this change will likely break unit tests and makes errors
less user-friendly. Consider using a stable, human-readable type label (e.g.,
"integer" vs "floating point") instead of `typeid(T).name()`.
##########
src/iceberg/util/string_util.h:
##########
@@ -67,17 +67,22 @@ class ICEBERG_EXPORT StringUtils {
}
template <typename T>
- static Result<T> ParseInt(std::string_view str) {
+ requires std::is_arithmetic_v<T> && (!std::same_as<T, bool>)
+ static Result<T> ParseNumber(std::string_view str) {
T value = 0;
auto [ptr, ec] = std::from_chars(str.data(), str.data() + str.size(),
value);
- if (ec == std::errc::invalid_argument) [[unlikely]] {
- return InvalidArgument("Failed to parse integer from string '{}':
invalid argument",
- str);
- } else if (ec == std::errc::result_out_of_range) [[unlikely]] {
+ if (ec == std::errc()) [[likely]] {
+ return value;
+ }
+ if (ec == std::errc::invalid_argument) {
+ return InvalidArgument("Failed to parse {} from string '{}': invalid
argument",
+ typeid(T).name(), str);
+ }
+ if (ec == std::errc::result_out_of_range) {
return InvalidArgument("Failed to parse {} from string '{}': value out
of range",
typeid(T).name(), str);
}
- return value;
+ std::unreachable();
}
Review Comment:
`ParseNumber` uses `std::same_as`, `std::is_arithmetic_v`, and
`std::unreachable()` but this header does not include the standard headers that
define them. Please add the required includes (typically `<concepts>`,
`<type_traits>`, and `<utility>`) to avoid relying on transitive includes and
potential build failures on different standard library implementations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
