wgtmac commented on code in PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#discussion_r4229588252
##########
src/iceberg/logging/log_level.h:
##########
@@ -66,7 +67,7 @@ constexpr std::string_view ToString(LogLevel level) noexcept {
case LogLevel::kOff:
return "off";
}
- std::unreachable();
+ ::iceberg::unreachable();
Review Comment:
Thanks. I checked the remaining uses: not all of them are in non-template
functions. Several public templates, such as those in `string_util.h` and
`visit_type.h`, still need a C++20-compatible `unreachable`. Also,
`LogLevel::ToString` is `constexpr`, so moving it to `.cc` would remove
constant evaluation. I would keep the compat wrapper for now and treat this as
a possible later cleanup.
##########
src/iceberg/update/snapshot_update.h:
##########
@@ -66,96 +77,124 @@ class ICEBERG_EXPORT SnapshotUpdate : public PendingUpdate
{
///
/// \param reporter The metrics reporter to use.
/// \return Reference to this for method chaining.
+#if ICEBERG_CXX20_COMPAT
Review Comment:
Thanks. Using the C++20 form in C++23 would change these setters to return
`SnapshotUpdate&`, which breaks existing derived fluent calls such as
`append.ToBranch(...).AppendFile(...)`. The macro selects a package-level API
shape: the default C++23 package keeps deducing this, while the compat package
uses base references. I would keep it unless we decide to break the existing
C++23 fluent API.
--
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]
