[V4] .. #53
[V4] .. #53
Conversation
📝 WalkthroughWalkthroughThe pull request reorganizes macro definitions by splitting the public Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@benchmark/src/libfork_benchmark/fib/lf_parts.cpp`:
- Around line 20-21: The thread-local TLS buffer is only byte-aligned causing
tls_stack::operator new to return misaligned memory when sp == buffer; update
the declaration of buffer to use the default-new alignment (e.g.,
alignas(lf::k_new_align)) so coroutine frames get proper alignment (keep sp as
thread_local static std::byte* sp = nullptr and only change the buffer
declaration to include alignas(lf::k_new_align)).
- Around line 30-34: The operator new implementation for tls_stack (the static
operator new(std::size_t) function) advances the stack pointer sp without
checking the 8KB buffer bounds; add a bounds check before updating sp that
ensures sp + align(sz) does not exceed the end of the TLS buffer and, on
overflow, calls std::terminate() to abort safely; also add a top-of-file include
for <exception> (or use import std; in C++20 modules) so std::terminate is
available.
In `@include/libfork/__impl/assume.hpp`:
- Around line 5-8: Update the Doxygen file tag at the top of
include/libfork/__impl/assume.hpp: replace the incorrect "@file compiler.hpp"
tag with "@file assume.hpp" (and adjust the brief description if needed) so the
generated documentation correctly labels this header; locate the tag in the file
header comment block and edit the tag text accordingly.
- Around line 26-30: The NDEBUG branch defines LF_ASSUME as a bare if that can
bind to an outer else; change its expansion to use the same do { ... }
while(false) wrapper as the debug branch so calls to LF_ASSUME(expr) are
statement-safe—i.e., wrap the existing conditional and ::std::unreachable() call
inside a do { if (!(expr)) { ::std::unreachable(); } } while (false) construct
to avoid dangling-else hazards.
In `@include/libfork/__impl/compiler.hpp`:
- Around line 3-10: The documentation in compiler.hpp claims the macros are
"standalone" but the file includes exception.hpp; either remove the `#include`
"libfork/__impl/exception.hpp" from compiler.hpp if the macros truly have no
dependency on exception types, or update the header comment in compiler.hpp to
explicitly state the dependency on libfork/__impl/exception.hpp (and why), so
the documentation and implementation match; locate the include and the
top-of-file comment in compiler.hpp to make the change.
In `@include/libfork/__impl/exception.hpp`:
- Line 69: The LF_RETHROW macro is defined with a typo as LF_TERLF_TERMINATE
which breaks compilation when exceptions are disabled; update the macro
definition for LF_RETHROW to call the correct LF_TERMINATE symbol (i.e., replace
LF_TERLF_TERMINATE with LF_TERMINATE) so the rethrow path uses the intended
terminate macro.
In `@src/core/promise.cxx`:
- Around line 163-167: The template promise_type<T, StackPolicy> is missing
coroutine API methods needed for non-void tasks; add a get_return_object() that
constructs and returns the corresponding task<T,...> (using frame and this
promise as needed) and implement return_value(T value) to store the returned
value into the promise's return_address (ensuring return_address is set by
get_return_object or the task constructor and that you memcpy/assign the value
into *return_address), while preserving existing members frame and StackPolicy;
reference promise_type, get_return_object, return_value, return_address, frame,
and task in your changes.
- Line 179: Fix the typo in the comment header "// =============== std
specialzation =============== //" by renaming "specialzation" to
"specialization" so it reads "// =============== std specialization
=============== //"; locate and update the comment in src/core/promise.cxx
(search for the string "std specialzation") to ensure the spelling is corrected
in the codebase.
🧹 Nitpick comments (1)
src/core/promise.cxx (1)
56-57: Inheriting fromstd::unique_ptris fragile.
std::unique_ptrdoes not have a virtual destructor, so inheriting from it (viaunique_promise) can lead to undefined behavior if ataskis deleted through a base pointer. Consider using composition instead or ensure this type is never used polymorphically.The Cppcheck syntax error is a false positive due to limited C++20 module support.
| thread_local static std::byte buffer[1024 * 8]; | ||
| thread_local static std::byte *sp = nullptr; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the actual file to confirm the current state
cat -n benchmark/src/libfork_benchmark/fib/lf_parts.cpp | head -30Repository: ConorWilliams/libfork
Length of output: 987
🏁 Script executed:
# Search for tls_stack implementation to understand operator new
fd -t f -name "*.hpp" -o -name "*.cpp" | xargs grep -l "tls_stack" | head -5Repository: ConorWilliams/libfork
Length of output: 295
🏁 Script executed:
# Search for k_new_align definition
rg "k_new_align" --type cpp --type hppRepository: ConorWilliams/libfork
Length of output: 92
🏁 Script executed:
# Check the coroutine frame or allocation requirements
rg "operator new" benchmark/src/libfork_benchmark/fib/ -A 10Repository: ConorWilliams/libfork
Length of output: 1804
🏁 Script executed:
# Search for where sp is initialized to buffer
rg "sp\s*=" benchmark/src/libfork_benchmark/fib/lf_parts.cpp -A 2Repository: ConorWilliams/libfork
Length of output: 218
🏁 Script executed:
# Find lf::k_new_align definition
rg "k_new_align" --type c --type cc --type cxx --type h -B 2 -A 2Repository: ConorWilliams/libfork
Length of output: 91
🏁 Script executed:
# Look for more context in the file - read the full operator new and any setup
sed -n '20,50p' benchmark/src/libfork_benchmark/fib/lf_parts.cppRepository: ConorWilliams/libfork
Length of output: 826
🏁 Script executed:
# Find k_new_align definition in libfork
find . -type f -name "*.cpp" -o -name "*.h" -o -name "*.hpp" -o -name "*.ixx" -o -name "*.cppm" | xargs rg "k_new_align\s*=" -B 2 -A 2Repository: ConorWilliams/libfork
Length of output: 1801
🏁 Script executed:
# Check what std::byte alignment is and coroutine frame requirements
rg "std::allocator|coroutine.*align|k_new_align" . -B 1 -A 1 | head -50Repository: ConorWilliams/libfork
Length of output: 568
🏁 Script executed:
# Look for libfork/core to understand the library structure
find . -name "core*" -type f | head -10Repository: ConorWilliams/libfork
Length of output: 183
🏁 Script executed:
# Check if there's any alignment initialization before sp is used
sed -n '1,100p' benchmark/src/libfork_benchmark/fib/lf_parts.cppRepository: ConorWilliams/libfork
Length of output: 2203
Align the TLS buffer to default-new alignment.
buffer is only byte-aligned by default, so tls_stack::operator new will return misaligned memory on the first allocation when sp = buffer. Coroutine frames require alignment to lf::k_new_align (typically 16+ bytes). Add explicit alignment to prevent undefined behavior.
♻️ Proposed fix
-thread_local static std::byte buffer[1024 * 8];
+alignas(lf::k_new_align) thread_local static std::byte buffer[1024 * 8];🤖 Prompt for AI Agents
In `@benchmark/src/libfork_benchmark/fib/lf_parts.cpp` around lines 20 - 21, The
thread-local TLS buffer is only byte-aligned causing tls_stack::operator new to
return misaligned memory when sp == buffer; update the declaration of buffer to
use the default-new alignment (e.g., alignas(lf::k_new_align)) so coroutine
frames get proper alignment (keep sp as thread_local static std::byte* sp =
nullptr and only change the buffer declaration to include
alignas(lf::k_new_align)).
| static auto operator new(std::size_t sz) -> void * { | ||
| auto *prev = sp; | ||
| sp += align(sz); | ||
| return prev; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "lf_parts.cpp" -o -name "*.cpp" | grep -E "lf_parts|libfork" | head -20Repository: ConorWilliams/libfork
Length of output: 550
🏁 Script executed:
cat -n benchmark/src/libfork_benchmark/fib/lf_parts.cpp | head -50Repository: ConorWilliams/libfork
Length of output: 1599
🏁 Script executed:
# Check what's in the includes at the top of the file - specifically looking for <new> or <cstdlib> which might provide std::terminate
head -10 benchmark/src/libfork_benchmark/fib/lf_parts.cpp
# Also check if std::terminate is in the C++ standard library headers already included
grep -E "^#include|^import" benchmark/src/libfork_benchmark/fib/lf_parts.cppRepository: ConorWilliams/libfork
Length of output: 368
🏁 Script executed:
# Let's also verify the buffer and sp are actually in scope for the tls_stack struct
sed -n '11,39p' benchmark/src/libfork_benchmark/fib/lf_parts.cppRepository: ConorWilliams/libfork
Length of output: 842
🏁 Script executed:
# Check for any error handling patterns or existing terminate usage in the codebase
rg "std::terminate|throw|assert" benchmark/src/libfork_benchmark/fib/ -A 2 -B 2 | head -30Repository: ConorWilliams/libfork
Length of output: 47
🏁 Script executed:
# Also check if there's any other stack overflow protection or allocation guards in the file
rg "buffer|sp\s|exhausted|overflow" benchmark/src/libfork_benchmark/fib/lf_parts.cpp -B 2 -A 2Repository: ConorWilliams/libfork
Length of output: 628
Add a bounds check to prevent TLS stack overflow.
tls_stack::operator new advances sp without guarding against buffer exhaustion. With deep recursion, sp can advance past the 8KB buffer, corrupting memory. Add a check before updating sp:
static auto operator new(std::size_t sz) -> void * {
auto *prev = sp;
- sp += align(sz);
+ auto *next = sp + align(sz);
+ if (next > buffer + sizeof(buffer)) {
+ std::terminate();
+ }
+ sp = next;
return prev;
}To make std::terminate accessible, add #include <exception> at the top (or import std; if using C++20 modules).
🤖 Prompt for AI Agents
In `@benchmark/src/libfork_benchmark/fib/lf_parts.cpp` around lines 30 - 34, The
operator new implementation for tls_stack (the static operator new(std::size_t)
function) advances the stack pointer sp without checking the 8KB buffer bounds;
add a bounds check before updating sp that ensures sp + align(sz) does not
exceed the end of the TLS buffer and, on overflow, calls std::terminate() to
abort safely; also add a top-of-file include for <exception> (or use import std;
in C++20 modules) so std::terminate is available.
| /** | ||
| * @file compiler.hpp | ||
| * | ||
| * @brief A collection of internal macros. |
There was a problem hiding this comment.
Fix the Doxygen @file tag to match the filename.
The header says compiler.hpp, but this file is assume.hpp, which will mislabel generated docs.
♻️ Proposed fix
- * `@file` compiler.hpp
+ * `@file` assume.hpp📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * @file compiler.hpp | |
| * | |
| * @brief A collection of internal macros. | |
| /** | |
| * `@file` assume.hpp | |
| * | |
| * `@brief` A collection of internal macros. |
🤖 Prompt for AI Agents
In `@include/libfork/__impl/assume.hpp` around lines 5 - 8, Update the Doxygen
file tag at the top of include/libfork/__impl/assume.hpp: replace the incorrect
"@file compiler.hpp" tag with "@file assume.hpp" (and adjust the brief
description if needed) so the generated documentation correctly labels this
header; locate the tag in the file header comment block and edit the tag text
accordingly.
| #ifdef NDEBUG | ||
| #define LF_ASSUME(expr) \ | ||
| if (!(expr)) { \ | ||
| ::std::unreachable(); \ | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -35 include/libfork/__impl/assume.hpp | tail -15Repository: ConorWilliams/libfork
Length of output: 1027
🏁 Script executed:
cat -n include/libfork/__impl/assume.hpp | head -40Repository: ConorWilliams/libfork
Length of output: 1829
🏁 Script executed:
# Check the full macro definition context
rg -A 10 "LF_ASSUME" include/libfork/__impl/assume.hpp | head -50Repository: ConorWilliams/libfork
Length of output: 991
🏁 Script executed:
rg "LF_ASSUME" --type cpp --type hpp -B 2 -A 2 | head -80Repository: ConorWilliams/libfork
Length of output: 92
🏁 Script executed:
rg "LF_ASSUME" -B 2 -A 2 | head -100Repository: ConorWilliams/libfork
Length of output: 1734
Wrap the NDEBUG macro body to avoid dangling-else hazards.
The NDEBUG branch expands to a bare if statement, which can cause unexpected binding to outer else clauses at call sites. The debug branch correctly uses do { } while(false) wrapping; apply the same pattern to the release macro for consistency and safety.
Proposed fix
`#ifdef` NDEBUG
`#define` LF_ASSUME(expr) \
- if (!(expr)) { \
- ::std::unreachable(); \
- }
+ do { \
+ if (!(expr)) { \
+ ::std::unreachable(); \
+ } \
+ } while (false)🤖 Prompt for AI Agents
In `@include/libfork/__impl/assume.hpp` around lines 26 - 30, The NDEBUG branch
defines LF_ASSUME as a bare if that can bind to an outer else; change its
expansion to use the same do { ... } while(false) wrapper as the debug branch so
calls to LF_ASSUME(expr) are statement-safe—i.e., wrap the existing conditional
and ::std::unreachable() call inside a do { if (!(expr)) { ::std::unreachable();
} } while (false) construct to avoid dangling-else hazards.
| #include "libfork/__impl/exception.hpp" | ||
|
|
||
| /** | ||
| * @file compiler.hpp | ||
| * | ||
| * @brief A collection of internal macros. | ||
| * | ||
| * These macros are standalone i.e. they can be used without importing/including anything else. |
There was a problem hiding this comment.
Documentation contradicts implementation.
The file includes exception.hpp at line 3, but the documentation on lines 8-10 claims these macros are "standalone" and "can be used without importing/including anything else." Either remove the include or update the documentation to reflect the actual dependency.
🤖 Prompt for AI Agents
In `@include/libfork/__impl/compiler.hpp` around lines 3 - 10, The documentation
in compiler.hpp claims the macros are "standalone" but the file includes
exception.hpp; either remove the `#include` "libfork/__impl/exception.hpp" from
compiler.hpp if the macros truly have no dependency on exception types, or
update the header comment in compiler.hpp to explicitly state the dependency on
libfork/__impl/exception.hpp (and why), so the documentation and implementation
match; locate the include and the top-of-file comment in compiler.hpp to make
the change.
| /** | ||
| * @brief Expands to ``throw`` if exceptions are enabled, otherwise terminates the program. | ||
| */ | ||
| #define LF_RETHROW LF_TERLF_TERMINATE("Tried to rethrow without compiler exceptions") |
There was a problem hiding this comment.
Typo causes compilation error: LF_TERLF_TERMINATE should be LF_TERMINATE.
This macro definition has a clear typo that will cause a compilation error when exceptions are disabled.
🐛 Proposed fix
- `#define` LF_RETHROW LF_TERLF_TERMINATE("Tried to rethrow without compiler exceptions")
+ `#define` LF_RETHROW LF_TERMINATE("Tried to rethrow without compiler exceptions")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #define LF_RETHROW LF_TERLF_TERMINATE("Tried to rethrow without compiler exceptions") | |
| `#define` LF_RETHROW LF_TERMINATE("Tried to rethrow without compiler exceptions") |
🤖 Prompt for AI Agents
In `@include/libfork/__impl/exception.hpp` at line 69, The LF_RETHROW macro is
defined with a typo as LF_TERLF_TERMINATE which breaks compilation when
exceptions are disabled; update the macro definition for LF_RETHROW to call the
correct LF_TERMINATE symbol (i.e., replace LF_TERLF_TERMINATE with LF_TERMINATE)
so the rethrow path uses the intended terminate macro.
| template <typename T, alloc_mixin StackPolicy> | ||
| struct promise_type : StackPolicy, mixin_frame { | ||
| frame_type frame; | ||
| T *return_address; | ||
| }; |
There was a problem hiding this comment.
Non-void promise_type is incomplete.
The general template for promise_type<T, StackPolicy> is missing required coroutine promise methods:
get_return_object()to construct thetask<T, ...>return_value(T)to store the coroutine's return value atreturn_address
This will cause compilation errors when attempting to use non-void coroutines.
Would you like me to help implement these missing methods?
🤖 Prompt for AI Agents
In `@src/core/promise.cxx` around lines 163 - 167, The template promise_type<T,
StackPolicy> is missing coroutine API methods needed for non-void tasks; add a
get_return_object() that constructs and returns the corresponding task<T,...>
(using frame and this promise as needed) and implement return_value(T value) to
store the returned value into the promise's return_address (ensuring
return_address is set by get_return_object or the task constructor and that you
memcpy/assign the value into *return_address), while preserving existing members
frame and StackPolicy; reference promise_type, get_return_object, return_value,
return_address, frame, and task in your changes.
|
|
||
| } // namespace lf | ||
|
|
||
| // =============== std specialzation =============== // |
There was a problem hiding this comment.
Minor typo: "specialzation" should be "specialization".
📝 Proposed fix
-// =============== std specialzation =============== //
+// =============== std specialization =============== //📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // =============== std specialzation =============== // | |
| // =============== std specialization =============== // |
🤖 Prompt for AI Agents
In `@src/core/promise.cxx` at line 179, Fix the typo in the comment header "//
=============== std specialzation =============== //" by renaming
"specialzation" to "specialization" so it reads "// =============== std
specialization =============== //"; locate and update the comment in
src/core/promise.cxx (search for the string "std specialzation") to ensure the
spelling is corrected in the codebase.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.