From 72c0a75c68279fcbcaa5a4180f63cab7828aeb6a Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Fri, 16 Oct 2026 18:20:38 +0800 Subject: [PATCH 1/4] fix: qualify Iceberg names in public error macros ICEBERG_RETURN_UNEXPECTED, and through it ICEBERG_ASSIGN_OR_RAISE, named Error without its namespace. ICEBERG_PRECHECK and ICEBERG_CHECK did the same with InvalidArgument and ValidationFailed, and the ICEBERG_BUILDER_* macros with ErrorKind. Those names resolve only inside namespace iceberg, so code that includes the installed headers could not use the macros. Qualify them with ::iceberg::, as the logging macros already do, and use ::iceberg:: in the exception macros as well. Co-Authored-By: Claude Opus 5.5 --- src/iceberg/test/CMakeLists.txt | 1 + src/iceberg/test/error_macros_test.cc | 108 ++++++++++++++++++++++++++ src/iceberg/util/error_collector.h | 16 ++-- src/iceberg/util/macros.h | 39 +++++----- 4 files changed, 139 insertions(+), 25 deletions(-) create mode 100644 src/iceberg/test/error_macros_test.cc diff --git a/src/iceberg/test/CMakeLists.txt b/src/iceberg/test/CMakeLists.txt index a86bc9087b..60f16b52ea 100644 --- a/src/iceberg/test/CMakeLists.txt +++ b/src/iceberg/test/CMakeLists.txt @@ -135,6 +135,7 @@ add_iceberg_test(util_test data_file_set_test.cc decimal_test.cc endian_test.cc + error_macros_test.cc expected_test.cc file_io_test.cc formatter_test.cc diff --git a/src/iceberg/test/error_macros_test.cc b/src/iceberg/test/error_macros_test.cc new file mode 100644 index 0000000000..edb6dff631 --- /dev/null +++ b/src/iceberg/test/error_macros_test.cc @@ -0,0 +1,108 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include +#include + +#include "iceberg/exception.h" +#include "iceberg/result.h" +#include "iceberg/test/matchers.h" +#include "iceberg/util/error_collector.h" +#include "iceberg/util/macros.h" + +namespace { + +using ::iceberg::HasErrorMessage; +using ::iceberg::HasValue; +using ::iceberg::IsError; +using ::iceberg::IsOk; + +iceberg::Result Parse(bool valid) { + if (!valid) { + return iceberg::InvalidArgument("Invalid input"); + } + return 42; +} + +iceberg::Result AddOne(bool valid) { + ICEBERG_ASSIGN_OR_RAISE(auto value, Parse(valid)); + return value + 1; +} + +iceberg::Status Validate(bool valid) { + ICEBERG_RETURN_UNEXPECTED(Parse(valid)); + return {}; +} + +iceberg::Status CheckRange(int value) { + ICEBERG_PRECHECK(value > 0, "Value must be positive, got {}", value); + ICEBERG_CHECK(value < 100, "Value must be below 100, got {}", value); + return {}; +} + +int AddOneOrThrow(bool valid) { + ICEBERG_ASSIGN_OR_THROW(auto value, Parse(valid)); + return value + 1; +} + +class TestBuilder : public iceberg::ErrorCollector { + public: + TestBuilder& Set(int value) { + ICEBERG_BUILDER_CHECK(value > 0, "Value must be positive, got {}", value); + ICEBERG_BUILDER_ASSIGN_OR_RETURN(value_, Parse(value < 100)); + ICEBERG_BUILDER_ASSIGN_OR_RETURN_WITH_ERROR(value_, Parse(value < 50), + "Value {} is too large", value); + return *this; + } + + private: + int value_ = 0; +}; + +} // namespace + +TEST(ErrorMacrosTest, AssignOrRaise) { + EXPECT_THAT(AddOne(true), HasValue(::testing::Eq(43))); + EXPECT_THAT(AddOne(false), IsError(iceberg::ErrorKind::kInvalidArgument)); +} + +TEST(ErrorMacrosTest, ReturnUnexpected) { + EXPECT_THAT(Validate(true), IsOk()); + EXPECT_THAT(Validate(false), HasErrorMessage("Invalid input")); +} + +TEST(ErrorMacrosTest, PrecheckAndCheck) { + EXPECT_THAT(CheckRange(1), IsOk()); + EXPECT_THAT(CheckRange(0), IsError(iceberg::ErrorKind::kInvalidArgument)); + EXPECT_THAT(CheckRange(100), IsError(iceberg::ErrorKind::kValidationFailed)); +} + +TEST(ErrorMacrosTest, AssignOrThrow) { + EXPECT_EQ(AddOneOrThrow(true), 43); + EXPECT_THROW(AddOneOrThrow(false), iceberg::IcebergError); +} + +TEST(ErrorMacrosTest, BuilderMacros) { + EXPECT_THAT(TestBuilder().Set(1).CheckErrors(), IsOk()); + EXPECT_THAT(TestBuilder().Set(0).CheckErrors(), + HasErrorMessage("Value must be positive, got 0")); + EXPECT_THAT(TestBuilder().Set(150).CheckErrors(), HasErrorMessage("Invalid input")); + EXPECT_THAT(TestBuilder().Set(70).CheckErrors(), + HasErrorMessage("Value 70 is too large")); +} diff --git a/src/iceberg/util/error_collector.h b/src/iceberg/util/error_collector.h index 9eb8de15a8..439ac980c6 100644 --- a/src/iceberg/util/error_collector.h +++ b/src/iceberg/util/error_collector.h @@ -31,6 +31,8 @@ namespace iceberg { +// The builder macros name Iceberg symbols with their full namespace, so builders outside +// namespace iceberg can use them. #define ICEBERG_BUILDER_RETURN_IF_ERROR(result) \ if (auto&& result_name = result; !result_name) [[unlikely]] { \ errors_.emplace_back(std::move(result_name.error())); \ @@ -49,7 +51,7 @@ namespace iceberg { #define ICEBERG_BUILDER_ASSIGN_OR_RETURN_WITH_ERROR_IMPL(result_name, lhs, rexpr, ...) \ auto&& result_name = (rexpr); \ if (!result_name) [[unlikely]] { \ - AddError(ErrorKind::kInvalidArgument, __VA_ARGS__); \ + AddError(::iceberg::ErrorKind::kInvalidArgument, __VA_ARGS__); \ return *this; \ } \ lhs = std::move(result_name.value()); @@ -58,12 +60,12 @@ namespace iceberg { ICEBERG_BUILDER_ASSIGN_OR_RETURN_WITH_ERROR_IMPL( \ ICEBERG_ASSIGN_OR_RAISE_NAME(result_, __COUNTER__), lhs, rexpr, __VA_ARGS__) -#define ICEBERG_BUILDER_CHECK(expr, ...) \ - do { \ - if (!(expr)) [[unlikely]] { \ - AddError(ErrorKind::kInvalidArgument, __VA_ARGS__); \ - return *this; \ - } \ +#define ICEBERG_BUILDER_CHECK(expr, ...) \ + do { \ + if (!(expr)) [[unlikely]] { \ + AddError(::iceberg::ErrorKind::kInvalidArgument, __VA_ARGS__); \ + return *this; \ + } \ } while (false) /// \brief Base class for collecting errors in the builder pattern. diff --git a/src/iceberg/util/macros.h b/src/iceberg/util/macros.h index ca481a9392..cd046ba6c8 100644 --- a/src/iceberg/util/macros.h +++ b/src/iceberg/util/macros.h @@ -21,15 +21,18 @@ /// \file iceberg/util/macros.h /// \brief Define common Iceberg utility macros. +/// +/// The macros name Iceberg symbols with their full namespace, so code outside namespace +/// iceberg can use them. #include #include "iceberg/exception.h" #include "iceberg/result.h" -#define ICEBERG_RETURN_UNEXPECTED(expr) \ - if (auto&& result_name = expr; !result_name) [[unlikely]] { \ - return iceberg::unexpected(result_name.error()); \ +#define ICEBERG_RETURN_UNEXPECTED(expr) \ + if (auto&& result_name = expr; !result_name) [[unlikely]] { \ + return ::iceberg::unexpected<::iceberg::Error>(result_name.error()); \ } #define ICEBERG_ASSIGN_OR_RAISE_IMPL(result_name, lhs, rexpr) \ @@ -49,26 +52,26 @@ #define ICEBERG_DCHECK(expr, message) assert((expr) && (message)) // Macro for precondition checks, usually used for function arguments -#define ICEBERG_PRECHECK(expr, ...) \ - do { \ - if (!(expr)) [[unlikely]] { \ - return InvalidArgument(__VA_ARGS__); \ - } \ +#define ICEBERG_PRECHECK(expr, ...) \ + do { \ + if (!(expr)) [[unlikely]] { \ + return ::iceberg::InvalidArgument(__VA_ARGS__); \ + } \ } while (0) // Macro for state checks, usually used for unexpected states -#define ICEBERG_CHECK(expr, ...) \ - do { \ - if (!(expr)) [[unlikely]] { \ - return ValidationFailed(__VA_ARGS__); \ - } \ +#define ICEBERG_CHECK(expr, ...) \ + do { \ + if (!(expr)) [[unlikely]] { \ + return ::iceberg::ValidationFailed(__VA_ARGS__); \ + } \ } while (0) -#define ERROR_TO_EXCEPTION(error) \ - if (error.kind == iceberg::ErrorKind::kInvalidExpression) { \ - throw iceberg::ExpressionError(error.message); \ - } else { \ - throw iceberg::IcebergError(error.message); \ +#define ERROR_TO_EXCEPTION(error) \ + if (error.kind == ::iceberg::ErrorKind::kInvalidExpression) { \ + throw ::iceberg::ExpressionError(error.message); \ + } else { \ + throw ::iceberg::IcebergError(error.message); \ } #define ICEBERG_THROW_NOT_OK(expr) \ From 81321d165139b389101e252047d4068031c923f0 Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Tue, 20 Oct 2026 08:25:43 +0800 Subject: [PATCH 2/4] fix: qualify IcebergError in ICEBERG_CHECK_OR_DIE Co-Authored-By: Claude Opus 5.5 --- src/iceberg/exception.h | 10 ++++---- src/iceberg/test/error_macros_test.cc | 37 ++++++++++++++++++++++----- 2 files changed, 36 insertions(+), 11 deletions(-) diff --git a/src/iceberg/exception.h b/src/iceberg/exception.h index 0333eb12ac..714a21bdb0 100644 --- a/src/iceberg/exception.h +++ b/src/iceberg/exception.h @@ -44,11 +44,11 @@ class ICEBERG_EXPORT ExpressionError : public IcebergError { explicit ExpressionError(const std::string& what) : IcebergError(what) {} }; -#define ICEBERG_CHECK_OR_DIE(condition, ...) \ - do { \ - if (!(condition)) [[unlikely]] { \ - throw iceberg::IcebergError(std::format(__VA_ARGS__)); \ - } \ +#define ICEBERG_CHECK_OR_DIE(condition, ...) \ + do { \ + if (!(condition)) [[unlikely]] { \ + throw ::iceberg::IcebergError(std::format(__VA_ARGS__)); \ + } \ } while (0) } // namespace iceberg diff --git a/src/iceberg/test/error_macros_test.cc b/src/iceberg/test/error_macros_test.cc index edb6dff631..f0b1f53eb0 100644 --- a/src/iceberg/test/error_macros_test.cc +++ b/src/iceberg/test/error_macros_test.cc @@ -33,24 +33,30 @@ using ::iceberg::HasValue; using ::iceberg::IsError; using ::iceberg::IsOk; -iceberg::Result Parse(bool valid) { +// The macros expand here, in a namespace with its own nested iceberg namespace, so a +// macro that names iceberg::X instead of ::iceberg::X fails to compile. +namespace caller { + +namespace iceberg {} + +::iceberg::Result Parse(bool valid) { if (!valid) { - return iceberg::InvalidArgument("Invalid input"); + return ::iceberg::InvalidArgument("Invalid input"); } return 42; } -iceberg::Result AddOne(bool valid) { +::iceberg::Result AddOne(bool valid) { ICEBERG_ASSIGN_OR_RAISE(auto value, Parse(valid)); return value + 1; } -iceberg::Status Validate(bool valid) { +::iceberg::Status Validate(bool valid) { ICEBERG_RETURN_UNEXPECTED(Parse(valid)); return {}; } -iceberg::Status CheckRange(int value) { +::iceberg::Status CheckRange(int value) { ICEBERG_PRECHECK(value > 0, "Value must be positive, got {}", value); ICEBERG_CHECK(value < 100, "Value must be below 100, got {}", value); return {}; @@ -61,7 +67,12 @@ int AddOneOrThrow(bool valid) { return value + 1; } -class TestBuilder : public iceberg::ErrorCollector { +int CheckPositiveOrDie(int value) { + ICEBERG_CHECK_OR_DIE(value > 0, "Value must be positive, got {}", value); + return value; +} + +class TestBuilder : public ::iceberg::ErrorCollector { public: TestBuilder& Set(int value) { ICEBERG_BUILDER_CHECK(value > 0, "Value must be positive, got {}", value); @@ -75,6 +86,15 @@ class TestBuilder : public iceberg::ErrorCollector { int value_ = 0; }; +} // namespace caller + +using caller::AddOne; +using caller::AddOneOrThrow; +using caller::CheckPositiveOrDie; +using caller::CheckRange; +using caller::TestBuilder; +using caller::Validate; + } // namespace TEST(ErrorMacrosTest, AssignOrRaise) { @@ -98,6 +118,11 @@ TEST(ErrorMacrosTest, AssignOrThrow) { EXPECT_THROW(AddOneOrThrow(false), iceberg::IcebergError); } +TEST(ErrorMacrosTest, CheckOrDie) { + EXPECT_EQ(CheckPositiveOrDie(1), 1); + EXPECT_THROW(CheckPositiveOrDie(0), iceberg::IcebergError); +} + TEST(ErrorMacrosTest, BuilderMacros) { EXPECT_THAT(TestBuilder().Set(1).CheckErrors(), IsOk()); EXPECT_THAT(TestBuilder().Set(0).CheckErrors(), From f08c3a725bd15138523ecd83f1947c9438e0dc32 Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Tue, 20 Oct 2026 23:13:57 +0800 Subject: [PATCH 3/4] refactor: move error macros out of namespace iceberg Co-Authored-By: Claude Opus 5.5 --- src/iceberg/exception.h | 4 ++-- src/iceberg/util/error_collector.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/iceberg/exception.h b/src/iceberg/exception.h index 714a21bdb0..ba288715cc 100644 --- a/src/iceberg/exception.h +++ b/src/iceberg/exception.h @@ -44,11 +44,11 @@ class ICEBERG_EXPORT ExpressionError : public IcebergError { explicit ExpressionError(const std::string& what) : IcebergError(what) {} }; +} // namespace iceberg + #define ICEBERG_CHECK_OR_DIE(condition, ...) \ do { \ if (!(condition)) [[unlikely]] { \ throw ::iceberg::IcebergError(std::format(__VA_ARGS__)); \ } \ } while (0) - -} // namespace iceberg diff --git a/src/iceberg/util/error_collector.h b/src/iceberg/util/error_collector.h index 439ac980c6..d8834f284b 100644 --- a/src/iceberg/util/error_collector.h +++ b/src/iceberg/util/error_collector.h @@ -29,8 +29,6 @@ #include "iceberg/iceberg_export.h" #include "iceberg/result.h" -namespace iceberg { - // The builder macros name Iceberg symbols with their full namespace, so builders outside // namespace iceberg can use them. #define ICEBERG_BUILDER_RETURN_IF_ERROR(result) \ @@ -68,6 +66,8 @@ namespace iceberg { } \ } while (false) +namespace iceberg { + /// \brief Base class for collecting errors in the builder pattern. /// /// This class equips builders with error accumulation capabilities to make it easy From e2b82d6119188a446a0a5b800c742d0ceb74af05 Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Tue, 20 Oct 2026 23:23:31 +0800 Subject: [PATCH 4/4] refactor: define ICEBERG_CHECK_OR_DIE before namespace iceberg Co-Authored-By: Claude Opus 5.5 --- src/iceberg/exception.h | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/iceberg/exception.h b/src/iceberg/exception.h index ba288715cc..a182cb0d82 100644 --- a/src/iceberg/exception.h +++ b/src/iceberg/exception.h @@ -30,6 +30,13 @@ #include "iceberg/iceberg_export.h" +#define ICEBERG_CHECK_OR_DIE(condition, ...) \ + do { \ + if (!(condition)) [[unlikely]] { \ + throw ::iceberg::IcebergError(std::format(__VA_ARGS__)); \ + } \ + } while (0) + namespace iceberg { /// \brief Base exception class for exceptions thrown by the Iceberg library. @@ -45,10 +52,3 @@ class ICEBERG_EXPORT ExpressionError : public IcebergError { }; } // namespace iceberg - -#define ICEBERG_CHECK_OR_DIE(condition, ...) \ - do { \ - if (!(condition)) [[unlikely]] { \ - throw ::iceberg::IcebergError(std::format(__VA_ARGS__)); \ - } \ - } while (0)