From f07a17ce2942b0cc3664c7cb21339e4ddff2b2e6 Mon Sep 17 00:00:00 2001 From: Niels Lohmann Date: Wed, 30 Sep 2026 18:19:33 +0200 Subject: [PATCH] Document the vendored doctest.h patch; fix stale doctest_compatibility.h comments tests/thirdparty/doctest/doctest.h is doctest 2.4.12, imported in #4771. Two weeks later, #4801 hand-edited translateActiveException() to declare "String res;" inside the translator loop instead of before it, so a translator that does not match does not leave a previous translator's result in "res" for the next iteration to see. Nothing recorded this, so re-vendoring doctest.h from upstream would silently drop the fix. Add a comment at the patched site naming the version, the PR and the reason, so a future re-vendor knows to re-apply it. Also fix two stale comments in doctest_compatibility.h: - The DOCTEST_THREAD_LOCAL comment referenced Xcode 6/7, which is no longer supported; reword it to explain why the define must stay regardless (it keeps doctest's own thread_local usage out of the way of the same Clang/MinGW crash that JSON_NO_THREAD_LOCAL works around in the library, see ci_test_no_thread_local). - The include's comment justified it with tests that define "private" as "public"; no test under tests/src does that any more (removed by #2352). Reword the comment instead of dropping the include, since confirming it is safe to drop needs the full CI matrix including MSVC 2015+. Verified locally that tests/src/unit-readme.cpp still builds and passes 17/17 with these headers. Closes #5714 item 9. Signed-off-by: Niels Lohmann --- tests/thirdparty/doctest/doctest.h | 6 ++++++ tests/thirdparty/doctest/doctest_compatibility.h | 16 ++++++++++++---- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/tests/thirdparty/doctest/doctest.h b/tests/thirdparty/doctest/doctest.h index 2a29eb40b..06e01255f 100644 --- a/tests/thirdparty/doctest/doctest.h +++ b/tests/thirdparty/doctest/doctest.h @@ -4498,6 +4498,12 @@ namespace { String translateActiveException() { #ifndef DOCTEST_CONFIG_NO_EXCEPTIONS auto& translators = getExceptionTranslators(); + // nlohmann/json locally patches upstream doctest 2.4.12 here: "res" was + // declared once before the loop and reused by every iteration; move it + // inside the loop so each translator gets a freshly default-constructed + // "res" instead of one a previous, non-matching translator may have + // written to (see nlohmann/json#4801). Re-apply this hunk when + // re-vendoring doctest.h from upstream. for(auto& curr : translators) { String res; if (curr->translate(res)) diff --git a/tests/thirdparty/doctest/doctest_compatibility.h b/tests/thirdparty/doctest/doctest_compatibility.h index e056a824d..153354683 100644 --- a/tests/thirdparty/doctest/doctest_compatibility.h +++ b/tests/thirdparty/doctest/doctest_compatibility.h @@ -2,7 +2,12 @@ #define DOCTEST_COMPATIBILITY #define DOCTEST_CONFIG_VOID_CAST_EXPRESSIONS -#define DOCTEST_THREAD_LOCAL // enable single-threaded builds on XCode 6/7 - https://github.com/onqtam/doctest/issues/172 +#define DOCTEST_THREAD_LOCAL // make doctest itself avoid thread_local - https://github.com/onqtam/doctest/issues/172 + // (Xcode 6/7, for which this define was originally added, is no longer + // supported, but the define must stay: it keeps doctest itself working the + // same way as JSON_NO_THREAD_LOCAL makes the library behave, for the same + // Clang/MinGW crash - see ci_test_no_thread_local in cmake/ci.cmake and + // include/nlohmann/detail/macro_scope.hpp) #include "doctest.h" // Catch doesn't require a semicolon after CAPTURE but doctest does @@ -26,9 +31,12 @@ #define CHECK_THROWS_WITH_STD_STR(expr, str) \ CHECK_THROWS_WITH_STD_STR_IMPL(expr, str, DOCTEST_ANONYMOUS(DOCTEST_STD_STRING_)) -// included here because for some tests in the json repository private is defined as -// public and if no STL header is included before that then in the json include when STL -// stuff is included the MSVC STL complains (errors) that C++ keywords are being redefined +// No test under tests/src still defines "private" as "public" (the last one was removed +// by #2352); this include predates that removal and stayed in case a test that does so +// is reintroduced, since it must come before in that case (MSVC's STL +// errors that C++ keywords are being redefined if an STL header pulled in indirectly by +// the json include is the first one to see the #define). Keep it: dropping it would need +// the full CI matrix, including MSVC 2015+, to confirm nothing still depends on it. #include // Catch does this by default