From 9cf7211bcc199c71a5664459ef47543c5bbb35a4 Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Wed, 25 Feb 2026 22:55:10 -0800 Subject: [PATCH 1/4] fix: Refactor --- REFACTORING_PLAN.md | 71 +++++++++++++++++++++++++++++++++++++++++++++ include/c2pa.hpp | 25 +++++----------- src/c2pa.cpp | 44 +++++++--------------------- 3 files changed, 89 insertions(+), 51 deletions(-) create mode 100644 REFACTORING_PLAN.md diff --git a/REFACTORING_PLAN.md b/REFACTORING_PLAN.md new file mode 100644 index 00000000..d1dd4559 --- /dev/null +++ b/REFACTORING_PLAN.md @@ -0,0 +1,71 @@ +# Refactoring Plan: Settings & Context Handling + +## Context + +The `Settings`, `Context`, `ContextBuilder`, and stream wrapper classes in `c2pa.hpp`/`c2pa.cpp` share substantial boilerplate for RAII pointer management, move semantics, and construction patterns. This plan identifies internal refactoring opportunities that preserve the public API while improving consistency, reducing duplication, and tightening encapsulation. + +--- + +## Refactoring 1: Normalize Move Semantics to `std::exchange` + +**Problem:** Six classes implement the same move-constructor/move-assignment/destructor pattern, but inconsistently — some use `std::exchange`, others manually null pointers; some null-check before `c2pa_free()`, others don't (both correct, but inconsistent). + +**Change:** Standardize all to: +```cpp +// Move ctor: use std::exchange +Class(Class&& other) noexcept : ptr(std::exchange(other.ptr, nullptr)) {} +// Move assign: no null check (c2pa_free handles nullptr) +Class& operator=(Class&& other) noexcept { + if (this != &other) { c2pa_free(ptr); ptr = std::exchange(other.ptr, nullptr); } + return *this; +} +// Destructor: no null check +~Class() noexcept { c2pa_free(ptr); } +``` + +**Affected classes:** Context, ContextBuilder (in `c2pa.cpp`), Reader, Signer, Builder (inline in `c2pa.hpp`) + +**Risk:** Low — behavior identical since `c2pa_free(nullptr)` is safe. + +--- + +## Refactoring 2: Delegate `Context(const Settings&)` to ContextBuilder + +**Problem:** `Context::Context(const Settings&)` at `c2pa.cpp:381-398` manually creates a `C2paContextBuilder*`, sets settings, and builds — duplicating what `ContextBuilder` already does with identical error handling. + +**Change:** Replace with delegation: +```cpp +Context::Context(const Settings& settings) + : Context(ContextBuilder().with_settings(settings).create_context()) {} +``` + +This mirrors the existing `Context(const std::string& json)` which already delegates through `Settings`. + +**Files:** `c2pa.cpp` only (lines 381-398) +**Risk:** Low — ContextBuilder implements identical logic. + + +## Files to Modify + +| File | Refactorings | +|------|-------------| +| [c2pa.hpp](include/c2pa.hpp) | 1, 3, 4, 5 | +| [c2pa.cpp](src/c2pa.cpp) | 1, 2, 3, 4, 5 | + +## Implementation Order + +1. **Refactoring 1** — Normalize `std::exchange` (smallest, zero risk, establishes consistency) +2. **Refactoring 2** — Context(Settings&) delegation (small, self-contained) + +Each step should be a separate commit. Run the full test suite after each. + +## Verification + +```bash +cd build && cmake --build . && ctest --output-on-failure +``` + +Tests to watch: +- `context.test.cpp` — covers Settings/Context/ContextBuilder move semantics, validation, signing +- `reader.test.cpp` — covers Reader construction paths +- `builder.test.cpp` — covers Builder construction and signing diff --git a/include/c2pa.hpp b/include/c2pa.hpp index 3f70dad6..92da6a2b 100644 --- a/include/c2pa.hpp +++ b/include/c2pa.hpp @@ -638,21 +638,17 @@ namespace c2pa Reader& operator=(const Reader&) = delete; Reader(Reader&& other) noexcept - : c2pa_reader(other.c2pa_reader), + : c2pa_reader(std::exchange(other.c2pa_reader, nullptr)), owned_stream(std::move(other.owned_stream)), cpp_stream(std::move(other.cpp_stream)) { - other.c2pa_reader = nullptr; } Reader& operator=(Reader&& other) noexcept { if (this != &other) { - if (c2pa_reader != nullptr) { - c2pa_free(c2pa_reader); - } - c2pa_reader = other.c2pa_reader; + c2pa_free(c2pa_reader); + c2pa_reader = std::exchange(other.c2pa_reader, nullptr); owned_stream = std::move(other.owned_stream); cpp_stream = std::move(other.cpp_stream); - other.c2pa_reader = nullptr; } return *this; } @@ -754,15 +750,13 @@ namespace c2pa /// @brief Move constructor. /// @param other Signer to move from. - Signer(Signer&& other) noexcept : signer(other.signer) { - other.signer = nullptr; + Signer(Signer&& other) noexcept : signer(std::exchange(other.signer, nullptr)) { } Signer& operator=(Signer&& other) noexcept { if (this != &other) { c2pa_free(signer); - signer = other.signer; - other.signer = nullptr; + signer = std::exchange(other.signer, nullptr); } return *this; } @@ -810,16 +804,13 @@ namespace c2pa Builder& operator=(const Builder&) = delete; - Builder(Builder&& other) noexcept : builder(other.builder) { - other.builder = nullptr; + Builder(Builder&& other) noexcept : builder(std::exchange(other.builder, nullptr)) { } Builder& operator=(Builder&& other) noexcept { if (this != &other) { - if (builder != nullptr) - c2pa_free(builder); - builder = other.builder; - other.builder = nullptr; + c2pa_free(builder); + builder = std::exchange(other.builder, nullptr); } return *this; } diff --git a/src/c2pa.cpp b/src/c2pa.cpp index ac5bda31..d70ee402 100644 --- a/src/c2pa.cpp +++ b/src/c2pa.cpp @@ -378,44 +378,24 @@ inline std::vector to_byte_vector(const unsigned char* data, int6 } } - Context::Context(const Settings& settings) : context(nullptr) { - if (!settings.is_valid()) { - throw C2paException("Settings object is invalid"); - } - auto builder = c2pa_context_builder_new(); - if (!builder) { - throw C2paException("Failed to create Context builder"); - } - if (c2pa_context_builder_set_settings(builder, settings.c_settings()) != 0) { - c2pa_free(builder); - throw C2paException(); - } - // The C API consumes the builder on build - context = c2pa_context_builder_build(builder); - if (!context) { - throw C2paException("Failed to build context"); - } + Context::Context(const Settings& settings) + : Context(ContextBuilder().with_settings(settings).create_context()) { } - Context::Context(Context&& other) noexcept : context(other.context) { - other.context = nullptr; + Context::Context(Context&& other) noexcept + : context(std::exchange(other.context, nullptr)) { } Context& Context::operator=(Context&& other) noexcept { if (this != &other) { - if (context) { - c2pa_free(context); - } - context = other.context; - other.context = nullptr; + c2pa_free(context); + context = std::exchange(other.context, nullptr); } return *this; } Context::~Context() noexcept { - if (context) { - c2pa_free(context); - } + c2pa_free(context); } C2paContext* Context::c_context() const noexcept { @@ -443,18 +423,14 @@ inline std::vector to_byte_vector(const unsigned char* data, int6 Context::ContextBuilder& Context::ContextBuilder::operator=(ContextBuilder&& other) noexcept { if (this != &other) { - if (context_builder) { - c2pa_free(context_builder); - } + c2pa_free(context_builder); context_builder = std::exchange(other.context_builder, nullptr); } return *this; } Context::ContextBuilder::~ContextBuilder() noexcept { - if (context_builder) { - c2pa_free(context_builder); - } + c2pa_free(context_builder); } bool Context::ContextBuilder::is_valid() const noexcept { @@ -497,7 +473,7 @@ inline std::vector to_byte_vector(const unsigned char* data, int6 throw C2paException("ContextBuilder is invalid (moved from)"); } - // The C API consumes the builder on build + // The C API consumes the context builder on build C2paContext* ctx = c2pa_context_builder_build(context_builder); if (!ctx) { throw C2paException("Failed to build context"); From 1d8679b9c20161548b613545db82a64a7e119248 Mon Sep 17 00:00:00 2001 From: tmathern <60901087+tmathern@users.noreply.github.com> Date: Wed, 25 Feb 2026 22:56:18 -0800 Subject: [PATCH 2/4] Delete REFACTORING_PLAN.md --- REFACTORING_PLAN.md | 71 --------------------------------------------- 1 file changed, 71 deletions(-) delete mode 100644 REFACTORING_PLAN.md diff --git a/REFACTORING_PLAN.md b/REFACTORING_PLAN.md deleted file mode 100644 index d1dd4559..00000000 --- a/REFACTORING_PLAN.md +++ /dev/null @@ -1,71 +0,0 @@ -# Refactoring Plan: Settings & Context Handling - -## Context - -The `Settings`, `Context`, `ContextBuilder`, and stream wrapper classes in `c2pa.hpp`/`c2pa.cpp` share substantial boilerplate for RAII pointer management, move semantics, and construction patterns. This plan identifies internal refactoring opportunities that preserve the public API while improving consistency, reducing duplication, and tightening encapsulation. - ---- - -## Refactoring 1: Normalize Move Semantics to `std::exchange` - -**Problem:** Six classes implement the same move-constructor/move-assignment/destructor pattern, but inconsistently — some use `std::exchange`, others manually null pointers; some null-check before `c2pa_free()`, others don't (both correct, but inconsistent). - -**Change:** Standardize all to: -```cpp -// Move ctor: use std::exchange -Class(Class&& other) noexcept : ptr(std::exchange(other.ptr, nullptr)) {} -// Move assign: no null check (c2pa_free handles nullptr) -Class& operator=(Class&& other) noexcept { - if (this != &other) { c2pa_free(ptr); ptr = std::exchange(other.ptr, nullptr); } - return *this; -} -// Destructor: no null check -~Class() noexcept { c2pa_free(ptr); } -``` - -**Affected classes:** Context, ContextBuilder (in `c2pa.cpp`), Reader, Signer, Builder (inline in `c2pa.hpp`) - -**Risk:** Low — behavior identical since `c2pa_free(nullptr)` is safe. - ---- - -## Refactoring 2: Delegate `Context(const Settings&)` to ContextBuilder - -**Problem:** `Context::Context(const Settings&)` at `c2pa.cpp:381-398` manually creates a `C2paContextBuilder*`, sets settings, and builds — duplicating what `ContextBuilder` already does with identical error handling. - -**Change:** Replace with delegation: -```cpp -Context::Context(const Settings& settings) - : Context(ContextBuilder().with_settings(settings).create_context()) {} -``` - -This mirrors the existing `Context(const std::string& json)` which already delegates through `Settings`. - -**Files:** `c2pa.cpp` only (lines 381-398) -**Risk:** Low — ContextBuilder implements identical logic. - - -## Files to Modify - -| File | Refactorings | -|------|-------------| -| [c2pa.hpp](include/c2pa.hpp) | 1, 3, 4, 5 | -| [c2pa.cpp](src/c2pa.cpp) | 1, 2, 3, 4, 5 | - -## Implementation Order - -1. **Refactoring 1** — Normalize `std::exchange` (smallest, zero risk, establishes consistency) -2. **Refactoring 2** — Context(Settings&) delegation (small, self-contained) - -Each step should be a separate commit. Run the full test suite after each. - -## Verification - -```bash -cd build && cmake --build . && ctest --output-on-failure -``` - -Tests to watch: -- `context.test.cpp` — covers Settings/Context/ContextBuilder move semantics, validation, signing -- `reader.test.cpp` — covers Reader construction paths -- `builder.test.cpp` — covers Builder construction and signing From 125ea2dbef338cc399723f6e62c8ce1c23d2bdf8 Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Wed, 25 Feb 2026 23:01:36 -0800 Subject: [PATCH 3/4] fix: Refactor --- include/c2pa.hpp | 1 + 1 file changed, 1 insertion(+) diff --git a/include/c2pa.hpp b/include/c2pa.hpp index 92da6a2b..193d0dce 100644 --- a/include/c2pa.hpp +++ b/include/c2pa.hpp @@ -40,6 +40,7 @@ #include #include #include +#include #include "c2pa.h" From 0cd3e88efbfc4fd022d6d7205e8d4be247847ee6 Mon Sep 17 00:00:00 2001 From: Tania Mathern Date: Wed, 25 Feb 2026 23:14:27 -0800 Subject: [PATCH 4/4] fix: Refactor --- src/c2pa.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/c2pa.cpp b/src/c2pa.cpp index d70ee402..963d8af6 100644 --- a/src/c2pa.cpp +++ b/src/c2pa.cpp @@ -287,10 +287,13 @@ inline std::vector to_byte_vector(const unsigned char* data, int6 /// This class is used to throw exceptions for errors encountered by the C2PA library via c2pa_error(). C2paException::C2paException() + : message_([]{ + auto result = c2pa_error(); + std::string msg = result ? std::string(result) : std::string(); + c2pa_free(result); + return msg; + }()) { - auto result = c2pa_error(); - message_ = result ? std::string(result) : std::string(); - c2pa_free(result); } C2paException::C2paException(std::string message) : message_(std::move(message))