TASK-072: extract hex_digit_value+unescape loop into shared header

Both src/http_utils.cpp and src/detail/http_request_impl.cpp kept
private copies of hex_digit_value and the core %HH/'+' decode loop,
each with an independent comment acknowledging the duplication.

Promote both helpers to src/httpserver/detail/unescape_helpers.hpp:
  - hex_digit_value: constexpr int, inlined
  - unescape_buf_raw: inline raw-buffer unescape (char*, size_t) -> size_t

http_unescape in http_utils.cpp is now a thin wrapper that calls
unescape_buf_raw and adds the trailing '\0' expected by callers of
the std::string* variant.

http_request_impl.cpp's anonymous http_unescape_raw is removed; its
sole caller (unescape_in_arena) now calls unescape_buf_raw directly.

Also addresses:
  - Rename unescape_in_arena parameter 'sink' -> 'value' and
    thread_local 'user_scratch' -> 'thread_value' for naming
    consistency. (code-simplifier-iter1-5)
  - Document the per-thread amortisation contract and arena-waste
    behaviour of the user-callback path. (performance-reviewer-iter1-2,
    performance-reviewer-iter1-3)
  - Collapse the long call-site comment in build_request_args to a
    single cross-reference line. (code-simplifier-iter1-7)

Addresses: code-simplifier-iter1-1 (critical), code-simplifier-iter1-2
(critical), code-simplifier-iter1-5 (major), performance-reviewer-iter1-2
(major), performance-reviewer-iter1-3 (major)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Author: etr

src/detail/http_request_impl.cpp

@@ -39,6 +39,7 @@
 
 #include "httpserver/detail/connection_state.hpp"
 #include "httpserver/detail/http_request_impl.hpp"
+#include "httpserver/detail/unescape_helpers.hpp"
 #include "httpserver/http_utils.hpp"
 
 namespace httpserver {
@@ -157,84 +158,53 @@ const http::header_view_map& http_request_impl::ensure_headerlike_cache(MHD_Valu
 
 namespace {
 
-// TASK-072: hex_digit_value matches src/http_utils.cpp's static helper.
-// Duplicated here (rather than promoted to a header) because both
-// copies are tiny and trivially constexpr-foldable; centralising it
-// would require either a header-only inline helper or an exposed
-// non-public symbol, neither of which justifies the surface area.
-constexpr int hex_digit_value(char c) noexcept {
-    if (c >= '0' && c <= '9') return c - '0';
-    if (c >= 'a' && c <= 'f') return c - 'a' + 10;
-    if (c >= 'A' && c <= 'F') return c - 'A' + 10;
-    return -1;
-}
-
-// TASK-072: raw-buffer unescape, mirrors http_unescape(std::string*)
-// byte-identically. Operates in-place on [data, data+size); returns
-// the new size after the transformation. The output is guaranteed to
-// be <= input length (the standard %HH / '+'->' ' replacement only
-// ever shrinks).
-inline std::size_t http_unescape_raw(char* data, std::size_t size) noexcept {
-    if (size == 0 || data[0] == '\0') return 0;
-    std::size_t rpos = 0;
-    std::size_t wpos = 0;
-    while (rpos < size && data[rpos] != '\0') {
-        switch (data[rpos]) {
-            case '+':
-                data[wpos++] = ' ';
-                ++rpos;
-                break;
-            case '%':
-                if (size > rpos + 2) {
-                    int hi = hex_digit_value(data[rpos + 1]);
-                    int lo = hex_digit_value(data[rpos + 2]);
-                    if (hi >= 0 && lo >= 0) {
-                        data[wpos++] =
-                            static_cast<char>((hi << 4) | lo);
-                        rpos += 3;
-                        break;
-                    }
-                }
-            // intentional fall through!
-            default:
-                data[wpos++] = data[rpos++];
-        }
-    }
-    return wpos;
-}
-
 // TASK-072: arena-routed unescape. The caller passes an arena-backed
 // pmr::string already holding the raw bytes; we run the unescape
 // transformation directly on the pmr::string's storage so no
 // global-heap allocation occurs on the warm path.
 //
-// When a user unescaper is registered, the public callback signature
-// is `void(std::string&)` (ABI-locked), so we route through a
-// thread_local std::string whose capacity amortises across all
-// requests on the same worker thread. The first request on a thread
-// grows it once; subsequent requests reuse the capacity (zero
-// per-request global-heap allocations on the warm path). The result
-// bytes are then assigned back into the arena-backed sink.
-inline void unescape_in_arena(std::pmr::string& sink,
+// Default path: calls httpserver::detail::unescape_buf_raw (shared
+// helper from unescape_helpers.hpp -- same algorithm as the
+// http_unescape wrapper in http_utils.cpp, one truth-source).
+//
+// User-callback path: the public callback signature is
+// `void(std::string&)` (ABI-locked), so we route through a
+// thread_local std::string (thread_value) whose capacity amortises
+// across all requests on the same worker thread.
+//
+// Amortisation contract: zero global-heap allocations on the warm
+// path per thread, *after* thread_value has grown to the peak value
+// length seen on that thread. The first call on a new thread, or the
+// first call that sees a value longer than any previous one, triggers
+// exactly one global-heap allocation to grow thread_value's buffer;
+// all subsequent calls at or below that length are allocation-free.
+// (performance-reviewer-iter1-2)
+//
+// Arena-waste note: when the user callback grows the value
+// (thread_value.size() > original value.size()), value's original
+// arena allocation is abandoned in the monotonic arena until
+// reset_arena(). This is an accepted trade-off of the ABI-locked
+// void(std::string&) signature; the arena is reset per request so
+// the waste is bounded by the request's total value bytes.
+// (performance-reviewer-iter1-3)
+inline void unescape_in_arena(std::pmr::string& value,
                               unescaper_ptr user_fn) {
-    if (sink.empty()) return;
+    if (value.empty()) return;
     if (user_fn == nullptr) {
         // Default %HH / '+' decode: run in-place on the arena
         // buffer, then truncate to the new size.
-        const std::size_t new_size = http_unescape_raw(
-            sink.data(), sink.size());
-        sink.resize(new_size);
+        const std::size_t new_size = httpserver::detail::unescape_buf_raw(
+            value.data(), value.size());
+        value.resize(new_size);
         return;
     }
     // User-callback path: route through a per-thread reusable
     // std::string scratch buffer so the warm-path cost is zero
-    // global-heap allocations. The thread_local lives for the
-    // worker thread's lifetime; capacity grows once per peak input
-    // size on the thread.
-    thread_local std::string user_scratch;
-    user_scratch.assign(sink.data(), sink.size());
-    user_fn(user_scratch);
-    sink.assign(user_scratch.data(), user_scratch.size());
+    // global-heap allocations after the first call on this thread.
+    thread_local std::string thread_value;
+    thread_value.assign(value.data(), value.size());
+    user_fn(thread_value);
+    value.assign(thread_value.data(), thread_value.size());
 }
 
 }  // namespace
@@ -274,26 +244,10 @@ MHD_Result http_request_impl::build_request_args(void* cls, MHD_ValueKind kind,
     }
     aa->accumulated_bytes += this_pair_bytes;
 
-    // TASK-072: route the unescape output into the per-connection
-    // arena. The destination pmr::string lives in the arena domain, so
-    // its bytes consume only arena memory (no global-heap allocation).
-    //
-    // Two paths:
-    //   - null user-unescaper: do the standard %HH / '+'->' '
-    //     transformation in-place on the arena-backed pmr::string via
-    //     http_unescape_raw (raw-buffer overload of the standard
-    //     unescape, defined in the anonymous namespace below).
-    //   - user-registered unescaper: the public callback signature
-    //     `void(std::string&)` is ABI-locked, so we pass it a
-    //     thread_local std::string scratch buffer; its capacity
-    //     amortises across requests on the same worker thread, leaving
-    //     the warm-path per-request cost at zero global-heap
-    //     allocations. The result is then copied into the arena-backed
-    //     pmr::string sink.
-    //
-    // The returned pmr::string's data is owned by the arena and lives
-    // until connection_state::reset_arena() runs (request completion),
-    // matching the TASK-018 string_view lifetime contract.
+    // Arena-routed unescape: see unescape_in_arena() above for path details.
+    // The returned pmr::string's data is owned by the arena and lives until
+    // connection_state::reset_arena() runs (request completion), matching
+    // the TASK-018 string_view lifetime contract.
     auto pmr_alloc = args.get_allocator();
     auto it = existing_it;
     if (it == args.end()) {
@@ -305,12 +259,10 @@ MHD_Result http_request_impl::build_request_args(void* cls, MHD_ValueKind kind,
     }
 
     // Allocate the destination pmr::string in the arena domain and
-    // copy the raw input bytes into it. The outer vector's allocator-
-    // propagating construct wires the inner pmr::string's allocator,
-    // so we pass (ptr, size) here. The unescape transformation then
+    // copy the raw input bytes into it. The unescape transformation
     // runs in-place on the arena-backed buffer.
-    auto& sink = it->second.emplace_back(val_sv.data(), val_sv.size());
-    unescape_in_arena(sink, aa->unescaper);
+    auto& arg_value_ref = it->second.emplace_back(val_sv.data(), val_sv.size());
+    unescape_in_arena(arg_value_ref, aa->unescaper);
     return MHD_YES;
 }
 

src/http_utils.cpp

@@ -57,6 +57,7 @@
 #include <vector>
 
 #include "httpserver/constants.hpp"
+#include "httpserver/detail/unescape_helpers.hpp"
 #include "httpserver/string_utilities.hpp"
 
 #if defined (__CYGWIN__)
@@ -302,49 +303,18 @@ std::string http_utils::sanitize_upload_filename(const std::string& filename) {
 // src/detail/ip_representation.cpp to keep this TU under the project
 // per-file LOC ceiling. See FILE_LOC_MAX in scripts/check-file-size.sh.
 
-static inline int hex_digit_value(char c) {
-    if (c >= '0' && c <= '9') return c - '0';
-    if (c >= 'a' && c <= 'f') return c - 'a' + 10;
-    if (c >= 'A' && c <= 'F') return c - 'A' + 10;
-    return -1;
-}
-
+// TASK-072 (code-simplifier-iter1-1/2): hex_digit_value and the core
+// unescape loop have been promoted to the shared internal header
+// src/httpserver/detail/unescape_helpers.hpp so that this TU and
+// src/detail/http_request_impl.cpp share one truth-source.
+// http_unescape is now a thin wrapper around unescape_buf_raw.
 size_t http_unescape(std::string* val) {
     if (val->empty()) return 0;
-
-    unsigned int rpos = 0;
-    unsigned int wpos = 0;
-
-    unsigned int size = val->size();
-
-    while (rpos < size && (*val)[rpos] != '\0') {
-        switch ((*val)[rpos]) {
-            case '+':
-                (*val)[wpos] = ' ';
-                wpos++;
-                rpos++;
-                break;
-            case '%':
-                if (size > rpos + 2) {
-                    int hi = hex_digit_value((*val)[rpos + 1]);
-                    int lo = hex_digit_value((*val)[rpos + 2]);
-                    if (hi >= 0 && lo >= 0) {
-                        (*val)[wpos] = static_cast<unsigned char>((hi << 4) | lo);
-                        wpos++;
-                        rpos += 3;
-                        break;
-                    }
-                }
-            // intentional fall through!
-            default:
-                (*val)[wpos] = (*val)[rpos];
-                wpos++;
-                rpos++;
-        }
-    }
-    (*val)[wpos] = '\0';  // add 0-terminator
-    val->resize(wpos);
-    return wpos;  // = strlen(val)
+    const std::size_t new_size =
+        httpserver::detail::unescape_buf_raw(val->data(), val->size());
+    (*val)[new_size] = '\0';  // add 0-terminator (std::string owns the byte)
+    val->resize(new_size);
+    return new_size;  // = strlen(val)
 }
 
 const std::string load_file(const std::string& filename) {

src/httpserver/detail/unescape_helpers.hpp

@@ -0,0 +1,94 @@
+/*
+     This file is part of libhttpserver
+     Copyright (C) 2011-2026 Sebastiano Merlino
+
+     This library is free software; you can redistribute it and/or
+     modify it under the terms of the GNU Lesser General Public
+     License as published by the Free Software Foundation; either
+     version 2.1 of the License, or (at your option) any later version.
+
+     This library is distributed in the hope that it will be useful,
+     but WITHOUT ANY WARRANTY; without even the implied warranty of
+     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+     Lesser General Public License for more details.
+
+     You should have received a copy of the GNU Lesser General Public
+     License along with this library; if not, write to the Free Software
+     Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301
+     USA
+*/
+
+// Internal-only helpers for URL percent-decoding.
+//
+// Shared between src/http_utils.cpp (http_unescape) and
+// src/detail/http_request_impl.cpp (arena-routed unescape).
+// Not part of the public API; not installed.
+//
+// TASK-072: extracted from the two translation units that previously
+// each maintained a private copy.  Having one source of truth ensures
+// that bug-fixes and RFC-3986 edge-case corrections propagate uniformly.
+// (code-simplifier-iter1-1, code-simplifier-iter1-2)
+
+#pragma once
+
+#include <cstddef>
+
+namespace httpserver {
+namespace detail {
+
+// Map a single hex character to its integer value [0, 15].
+// Returns -1 for any character that is not a valid hex digit.
+//
+// constexpr so callers in constant-expression contexts can fold the
+// table at compile time; trivially inlined at -O1 and above.
+[[nodiscard]] constexpr int hex_digit_value(char c) noexcept {
+    if (c >= '0' && c <= '9') return c - '0';
+    if (c >= 'a' && c <= 'f') return c - 'a' + 10;
+    if (c >= 'A' && c <= 'F') return c - 'A' + 10;
+    return -1;
+}
+
+// Percent-decode and '+'-to-space unescape a raw byte buffer in-place.
+//
+// Reads from [data, data+size), writes the decoded bytes back into the
+// same buffer starting at data[0], and returns the new (decoded) length.
+// The output length is always <= the input length because %HH sequences
+// (3 bytes) decode to 1 byte and '+' stays 1 byte.
+//
+// The loop stops at a '\0' byte or when rpos reaches size, whichever
+// comes first, so embedded null bytes in the input are treated as
+// terminators (matching the behaviour of the original http_unescape).
+//
+// The caller is responsible for adding any terminating '\0' after the
+// returned length if the buffer must be C-string-compatible.
+inline std::size_t unescape_buf_raw(char* data, std::size_t size) noexcept {
+    if (size == 0) return 0;
+    std::size_t rpos = 0;
+    std::size_t wpos = 0;
+    while (rpos < size && data[rpos] != '\0') {
+        switch (data[rpos]) {
+            case '+':
+                data[wpos++] = ' ';
+                ++rpos;
+                break;
+            case '%':
+                if (size > rpos + 2) {
+                    const int hi = hex_digit_value(data[rpos + 1]);
+                    const int lo = hex_digit_value(data[rpos + 2]);
+                    if (hi >= 0 && lo >= 0) {
+                        data[wpos++] =
+                            static_cast<char>((hi << 4) | lo);
+                        rpos += 3;
+                        break;
+                    }
+                }
+            // intentional fall through!
+            default:
+                data[wpos++] = data[rpos++];
+        }
+    }
+    return wpos;
+}
+
+}  // namespace detail
+}  // namespace httpserver