PingLiuPing commented on code in PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#discussion_r4119229470
##########
src/iceberg/util/error_collector.h:
##########
@@ -101,6 +104,13 @@ class ICEBERG_EXPORT ErrorCollector {
ErrorCollector(const ErrorCollector&) = default;
ErrorCollector& operator=(const ErrorCollector&) = default;
+// C++23 uses deducing `this` so that `return AddError(...)` keeps returning
the
Review Comment:
Thanks, this is intended actually as I want to limit the scope to the header
only.
But agree with you, there is duplication here.
My latest commit go with one API shape: `ErrorCollector` and
`SnapshotUpdate` now expose plain member functions returning the base type in
both C++20 and C++23. This removes the duplicated blocks, and the class
definition no longer differs between C++20 and C++23 translation units.
For derived-type fluent chaining: inside the repo, nothing relies on
derived-type fluent chaining. However, this is a breaking API change for C++23
users, so I'd like your call on whether it's acceptable here:
AddError(...) now returns ErrorCollector&. A user builder deriving from
`ErrorCollector` that does return AddError(...); in a method returning its own
type no longer compiles.
`SnapshotUpdate` setters now return SnapshotUpdate&, so a derived-class
method can no longer be chained after them.
--
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]
