PingLiuPing commented on issue #928:
URL: https://github.com/apache/iceberg-cpp/issues/928#issuecomment-5665339053
Thanks @mbasmanova for filing this issue.
`Result` and `Status` in `result.h` are exactly where the C++23 requirement
reaches consumers.
And thanks @wgtmac, @zhjwpku and @HuaHuaY for the quick and thoughtful
replies.
To be explicit about what we are asking for: we only want the **public
headers** to be portable. We are not proposing to lower the project's own C++
standard. iceberg-cpp would keep building as C++23; the `.cc` files and the
`*_internal.h` headers keep using C++23. Only the headers a consumer actually
include would hold a lower standard. This matches what @HuaHuaY asked for, a
wrapper rather than a downgrade.
Two options in this thread
1. **Portable C++ headers at a C++20 baseline** — @mbasmanova's proposal: a
vendored `expected` with the same interface behind `Result` and `Status`, with
the library itself staying on C++23. As @wgtmac pointed out, `<expected>` and
`<format>` alone are not enough for a consumer to actually include the headers,
so this also means removing other C++23 features from them. The result is a
C++20 baseline for the public headers.
2. **A C API**, as @zhjwpku suggested, in the style of RocksDB.
The two are not exclusive: a C API would also be the only thing that reaches
the C++17 and C++11 consumers. Compiling the public headers at C++17 gives
**32 of 205** passing, and the blockers are:
| feature | public headers using it |
| --- | --- |
| `std::span` in signatures | 31 |
| `requires` clauses | 15 |
| `std::format` / `<format>` | 11 |
| `concept` definitions | 8 |
| `std::remove_cvref_t` | 6 |
| `operator<=>` | 2 |
| `consteval` | 1 |
Reaching C++17 would mean more work.
I have a branch that go with option 1, and the complexity is modest: 48
files, **+264 / −136 lines** excluding the restored vendored file and its test:
- Restore `src/iceberg/expected.h` from #40 (removed by #139) and alias
`Result`
to it unconditionally. The library's own errors become
`iceberg::unexpected`.
- Replace `std::unreachable` in 11 public headers, `std::ranges::to` in 2 and
`std::byteswap` in 1.
- Drop deducing `this` from `TaskGroup` (ref-qualified overloads) and from
`ErrorCollector::AddError` (chaining on `*this`, which is already the
derived
type).
- Build `example/` as C++20 in CI, so a public header cannot silently pick
up a
C++23 feature again — it is already built on Linux, macOS and Windows
against
the installed package, which is exactly a consumer's view.
Verified locally: all 205 public headers compile standalone as C++20, the
library
still builds as C++23, and the test suites pass (1878 tests across
`util_test`,
`schema_test`, `table_test`, `expression_test`, `json_serde_test`),
including the
26 tests in the restored `expected_test.cc`.
Let me know if this is the right direction you want to go? Thanks.
--
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]
