Copilot commented on code in PR #960:
URL: https://github.com/apache/iceberg-cpp/pull/960#discussion_r4270937385
##########
src/iceberg/table.h:
##########
@@ -50,11 +51,13 @@ class ICEBERG_EXPORT Table : public
std::enable_shared_from_this<Table> {
/// string representation of identifier when empty.
/// \param[in] reporter Optional metrics reporter for this table. Defaults
to nullptr
/// (noop).
+ /// \param[in] labels Labels for this table, or nullptr when it has none.
static Result<std::shared_ptr<Table>> Make(
TableIdentifier identifier, std::shared_ptr<TableMetadata> metadata,
std::string metadata_location, std::shared_ptr<FileIO> io,
std::shared_ptr<Catalog> catalog, std::string full_name = "",
- std::shared_ptr<MetricsReporter> reporter = nullptr);
+ std::shared_ptr<MetricsReporter> reporter = nullptr,
+ std::unique_ptr<Labels> labels = nullptr);
Review Comment:
Adding a parameter to the exported `Table::Make` changes its mangled name
and is an ABI break for existing binaries, even with a default argument. If
this project aims to keep ABI stable, prefer keeping the original signature and
adding an overload (or a new `MakeWithLabels(...)`) that forwards to the new
implementation with `labels`, while leaving the old symbol intact.
##########
src/iceberg/table.h:
##########
@@ -226,6 +236,7 @@ class ICEBERG_EXPORT Table : public
std::enable_shared_from_this<Table> {
std::shared_ptr<Catalog> catalog_;
std::shared_ptr<MetricsReporter> reporter_;
std::unique_ptr<class TableMetadataCache> metadata_cache_;
+ const std::unique_ptr<const Labels> labels_;
Review Comment:
Making the `std::unique_ptr` itself `const` prevents move
construction/assignment of `Table` because `const std::unique_ptr<T>` is
neither copyable nor movable. Unless `Table` is intentionally required to be
non-movable (and enforced elsewhere), it’s safer to drop the top-level `const`
and keep only the pointee constness (`std::unique_ptr<const Labels> labels_`),
which still prevents mutation of the labels object while avoiding surprising
type restrictions.
--
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]
