Support etl::inplace_function #1046

jiangyilism · 2025-03-06T07:31:12Z

Provide similar api to std::function.

Current use case is mainly to hold a r-value lambda object, which etl::delegate is unsuitable since etl::delegate does not extend the lifetime of r-value lambda object.

Store a callable inside the inplace_function object's member field in a type-erasure way.
The member field, i.e. storage or buffer, has a compile-time fixed size.
The size is specified either by the macro ETL_INPLACE_FUNCTION_DEFAULT_CAPACITY or a non-type template parameter.

The implementation is inspired by:

SG14 inplace_function
folly::Function
function2

Summary by CodeRabbit

New Features

Introduced a fixed-capacity, type-erased callable wrapper supporting copy/move semantics and exception safety.
Added exception classes for callable invocation errors.
Added type traits for invocability detection with return type checking.

Tests

Added comprehensive tests for the new callable wrapper, covering various callable types and scenarios.
Introduced tests for the new invocability type traits.

Chores

Updated test build configuration to include new test files.

jiangyilism · 2025-03-06T07:54:16Z

include/etl/private/inplace_function_cpp14.h

+      using dtor_func_t = void (*)(storage_ptr_t);
+
+      const invoke_func_t       invoke_func{[](storage_ptr_t, Args&&...) -> R
+                                      { return R(); }};


std::function throws std::bad_function_call if operator() is called on an empty object while etl::inplace_function safely returns a default-constructed object in this case.
That means type typename R is required to be default-constructible.
void type is fine here ( https://timsong-cpp.github.io/cppwp/std14/expr.type.conv )
But this will not compile since std::is_default_constructible_v<Type_a> is false:

struct Type_a { Type_a(int32_t val) : val_(val) {} int32_t val_; }; etl::inplace_function<Type_a()> func0;

This deviation from std::function is by designed and also tested in test_inplace_function.cpp

In etl::delegate an ETL error is raised.
ETL_ASSERT(is_valid(), ETL_ERROR(delegate_uninitialised));

Also in etl::delegate there are call_if and call_or member functions that handle uninitialised objects.

Updated to make it raise etl::bad_inplace_function_call when an empty etl::inplace_function is invoked with operator() . (Similar to std::bad_function_call)
std::function does not support call_if() and call_or(). Do you suggest adding these 2 member func?

@jiangyilism I suggest adding them. It avoids having to check if the object is assigned anything, and helps moving from delegate.

clear() would also be nice to have for delegate compatibility.

jwellbelove · 2025-03-06T19:22:37Z

include/etl/private/inplace_function_cpp14.h

+https://github.com/ETLCPP/etl
+https://www.etlcpp.com
+
+Copyright(c) 2025 BMW AG


How close a copy is this of the implementations that 'inspired' you?
There may be copyright implications.

The closest open source implementation is SG14 inplace_function.
etl::inplace_function compiled object memory layout will be almost identical (except alignment).
The main differences:

etl is compatible with c++11 while others, including SG14, typically requires c++14

etl one has no member/non-member function swap(), operator==(nullptr_t), operator!=(nullptr_t) .

etl does not utilize aligned storage, which will be deprecated in c++23.

etl renames most of the class and variable names, except the name "inplace_function", which can be renamed too.

etl simplifies some template meta programming with etl type_traits header

move almost all stuffs in private namespace into private sections of classes.

jwellbelove · 2025-03-06T19:33:10Z

include/etl/private/inplace_function_cpp14.h

+      typename T,
+      typename C = etl::decay_t<T>,
+      typename = etl::enable_if_t<
+        !private_inplace_function::is_inplace_function<C>::value && etl::is_invocable_r<R, C&, Args...>::value>>


This class has limited usefulness as is restricted to STL and C++14 and above.
Many of the users of the ETL do not/cannot use the STL.

etl::is_invocable_r depends on ETL_USING_CPP11 but not STL.
I updated to make etl::inplace_function compatible to c++11.

denravonska · 2025-03-15T11:44:05Z

The missing R-value support blocks us from upgrading ETL. This will be great :)

BartolomeyKant · 2025-04-01T05:31:11Z

include/etl/private/inplace_function_cpp11.h

+      explicit ETL_CONSTEXPR vtable_t(etl::type_identity<T>) ETL_NOEXCEPT
+        : invoke_func{[](char* pobj, Args&&... args) -> R
+                      { return (*reinterpret_cast<T*>(pobj))(
+                          etl::forward<Args&&>(args)...); }},


I'm confused about this construction. Why etl::forward<Args&&> instead of etl::forward<Args> is used here?

Thanks. Fixed to etl::forward<Args> .

BartolomeyKant · 2025-04-01T05:40:16Z

include/etl/private/inplace_function_cpp11.h

+      ETL_ASSERT(vtable_ptr->invoke_func, ETL_ERROR(bad_inplace_function_call));
+      return vtable_ptr->invoke_func(
+        obj,
+        etl::forward<Args>(args)...);


Does conversion from Args... to Args&&... is working here?

For example I want to pass some struct Foo by value into inplace_function. Would it be copied three times?

First in call to this operator(), second in invoke_func call, and third in call to actual functor inside the invoke_func lambda?

I'm not an expert I'm just wondering how this should work.

Maybe std::move(args) should be used here, and then std::forward<Args>(args) in invoke_func lambda?

It is copied twice under gcc/clang -O0 , -O1 and -O2 . Using etl::move/std::move still copies twice.
A test case NoRedundentCopy is added to verify that during inplace_function::operator(), if the contained callable signature is:

call-by-value: then at most 2 copyings are made in inplace_function::operator().

call-by-reference: then no copying is made.

This example gives me one copy and one move https://godbolt.org/z/3YPMccbnz

Yes. And if the move constructor is not explicitly defined then copy constructor is called instead. NoRedundentCopy only checks the number of copy is less than 3 . It roughly checks that inplace_function is not copying too many times than expected. It does not guarantee the exact number of copying.

With gcc:

copy ctor not explicitly defined : copy twice

copy ctor =default : copy once

copy ctor explicitly defined : copy once

denravonska · 2025-04-03T06:29:37Z

Out of curiosity, what's using all the space? I had to bump the size to 48 bytes for an inplace_function<void(void *)> on Cortex-M33.

rolandreichweinbmw · 2025-04-03T07:14:35Z

Out of curiosity, what's using all the space? I had to bump the size to 48 bytes for an inplace_function<void(void *)> on Cortex-M33.

Depends on what you save inside. If it is just a "void f1(void*){}" it maybe fits?

But if it's a lambda capturing a number of arguments by value inside, it may be much more.

rolandreichweinbmw · 2025-04-03T07:16:18Z

John, can you please enable the CI workflows for this PR?

denravonska · 2025-04-03T07:20:41Z

Out of curiosity, what's using all the space? I had to bump the size to 48 bytes for an inplace_function<void(void *)> on Cortex-M33.

Depends on what you save inside. If it is just a "void f1(void*){}" it maybe fits?

But if it's a lambda capturing a number of arguments by value inside, it may be much more.

Actually, I accidentally compiled for host so it's 48 bytes on a 64 bit system which makes a bit more sense.

BartolomeyKant · 2025-04-03T07:44:58Z

64 bit system

What if ETL_INPLACE_FUNCTION_DEFAULT_CAPACITY would be calculated based on pointer size, like sizeof(void*)*4?

denravonska · 2025-04-03T07:59:51Z

include/etl/private/inplace_function_cpp11.h

+#include "../utility.h"
+
+#ifndef ETL_INPLACE_FUNCTION_DEFAULT_CAPACITY
+  #define ETL_INPLACE_FUNCTION_DEFAULT_CAPACITY 32


Can this be based on the target architecture? When building for 64 bit the bare minimum (void()) seems to be 40 bytes.

rolandreichweinbmw · 2025-04-03T08:03:08Z

64 bit system

What if ETL_INPLACE_FUNCTION_DEFAULT_CAPACITY would be calculated based on pointer size, like sizeof(void*)*4?

Similar to etl::delegate and etl::function, inplace_function has one single global size.

This one needs to store all possible closures in a whole project that uses ETL for a inplace_function template instance type. I.e. it depends.

E.g. for a simple etl::inplace_function<void(void *)> as mentioned by Denravonska, you might save different closure implementations, e.g.:

small: void f1(void*){}
big: [=]<void(void *)>(){a(b);c(d);e(f);g(h);}

I.e. needs to be adjusted if necessary for every project, to not waste memory.

I'm not sure if you can find a way to automatically calculate a global maximum size over all user locations. Finally, you can only find at link time (or after compiling all comile units) if you have captured all necessary source locations.

BartolomeyKant · 2025-04-03T08:18:25Z

This one needs to store all possible closures in a whole project that uses ETL

I agree with that, and it's the library user's responsibility to use only closures that fit or provide a large enough value to inplace_function Capacity.

But why did 32 was chosen for default?

For me the usual case closure captures some values by reference i.e. maximum 4 pointers for 32 byte storage on 64 bit system.
What if I had tested my application on system with 4 bytes pointers and capture 8 pointers in closure, and then moved to 8 byte sized pointer system?

@denravonska also asked about ETL_INPLACE_FUNCTION_DEFAULT_CAPACITY based on architecture.

I think solution based on sizeof(void*) is the simplest. The smaller pointers the smaller inplace_function and vice versa.

rolandreichweinbmw · 2025-04-03T08:40:34Z

Hi @BartolomeyKant and @denravonska , I think now I got your point. :-) And I agree that different defaults for e.g. 32 and 64 bit platforms can be provided.

Even though an individual ETL_INPLACE_FUNCTION_DEFAULT_CAPACITY will typically be part of a project's etl_profile.h anyway.

The original default of 32 covered our initial use cases in a 32 bit project.

Luckily, the compiler will tell you at build time if it doesn't fit, as shown in @denravonska 's example.

denravonska · 2025-04-03T10:39:03Z

Hi @BartolomeyKant and @denravonska , I think now I got your point. :-) And I agree that different defaults for e.g. 32 and 64 bit platforms can be provided.

Even though an individual ETL_INPLACE_FUNCTION_DEFAULT_CAPACITY will typically be part of a project's etl_profile.h anyway.

The original default of 32 covered our initial use cases in a 32 bit project.

Luckily, the compiler will tell you at build time if it doesn't fit, as shown in @denravonska 's example.

I thought about this a bit more. My original concern was that the default size was valid for 32 bit but failed to compile on 64 bit, so maybe the default should be the smallest size required to fit the simplest inplace function given the target architecture.

However this only applies to the default function. As soon as you do anything fancier you need to handle the size differences in the application anyway.

jiangyilism · 2025-04-03T14:21:44Z

Thanks for all the feedback.
@jwellbelove As you said in the slack channel discussion, your have a plan for a new fixed-storage function/delegate . Do you think I should drop this PR?

denravonska · 2025-04-03T15:02:54Z

Thanks for all the feedback. @jwellbelove As you said in the slack channel discussion, your have a plan for a new fixed-storage function/delegate . Do you think I should drop this PR?

He also said

I may decided to go with the etl::inplace_function instead.

I have no major quarrel with the implementation. The only request I have so far is a default size that works for both 32- and 64 bit systems.

jwellbelove · 2025-04-04T12:50:59Z

@jiangyilism No, keep it active for the time being. I'm in Greece at the moment, so I'll look at this functionality again when I get back next week.

jiangyilism · 2025-04-07T06:12:41Z

@denravonska , @BartolomeyKant
ETL_INPLACE_FUNCTION_DEFAULT_CAPACITY is changed to 8 * sizeof(void*) .

rolandreichweinbmw · 2025-04-07T11:39:40Z

test/test_inplace_function.cpp

+{
+  SUITE(test_inplace_function)
+  {
+    TEST(NoRedundentCopy)


NoRedundantCopy

denravonska

It would be a quality of life change with .call_if(). Consider

for(auto cb : callbacks) {
   if(cb) {
      cb();
   }
}

vs

for(auto cb : callbacks) {
   cb.call_if();
}

That, .call_or and a .clear would make it a lot more compatible with delegate.

jiangyilism · 2025-04-07T13:00:05Z

call_if

std::function does not support call_if, call_or, clear .
std::function and etl::inplace_function clear its content with operator=(nullptr_t) noexcept, which is now tested in HasValue test case.

If we want all etl::delegate api then it might be better to make inplace_function and delegate into a same class.
We need @jwellbelove 's feedback to continue with one of the directions:

extend inplace_function with api compatible with std::function. We can rename inplace_function though.
merge inplace_function into delegate
merge inplace_function (and delegate?) into etl::function
something else?

denravonska · 2025-04-07T13:14:51Z

call_if

std::function does not support call_if, call_or, clear . std::function and etl::inplace_function clear its content with operator=(nullptr_t) noexcept, which is now tested in HasValue test case.

If we want all etl::delegate api then it might be better to make inplace_function and delegate into a same class. We need @jwellbelove 's feedback to continue with one of the directions:

extend inplace_function with api compatible with std::function. We can rename inplace_function though.

merge inplace_function into delegate

merge inplace_function (and delegate?) into etl::function

something else?

I'm thinking more from the convenience perspective. I think a very common use case is going to be moving from delegate to inplace_function once you realize the limitations of the former and are willing to pay the cost of the latter. delegate has the luxury of not having a real counter part in STL so it can be a bit more free in its API.

I guess it all boils down to how true to the standard ETL wants to be in terms of adding functionality that's outside of the specification.

rolandreichweinbmw · 2025-04-07T13:55:38Z

I'd propose @jiangyilism 's option 1. in this way: Be close to std::function to minimize confusion, and if possible rename etl::inplace_function to etl::function. The latter of course only if @jwellbelove decides to abandon the old (already deprecated) etl::function.

jwellbelove · 2025-04-07T14:56:01Z

@rolandreichweinbmw The current etl::function (based on virtual functions) is indeed deprecated.
I have a complete std::function clone in a private branch, although it currently has the same limitation as etl::delegate when it comes to lambdas, which is why I 'parked' it for the time being. Its only real advantage over etl::delegate was that it had the same API as std::function (apart from lambdas) and could accept runtime function pointers.
It's possible I may be able to add runtime function pointers to etl::delegate without affecting constexpr compatibility.

I don't have any problem with adding functions that enhance the usability of an ETL class; making an exact clone of the STL was never my intention.

jwellbelove · 2025-04-07T15:02:46Z

std::function does not support call_if, call_or, clear .\nstd::function and etl::inplace_function clear its content with operator=(nullptr_t) noexcept

I've always treated the STL as inspiration for classes in the ETL, not as a rigid specification for what can and can't be included.

jwellbelove · 2025-04-07T15:08:06Z

I have limited options to review this PR at the moment, as I'm on holiday in Kos. I'm not quite sure when I will get back, as the Greek air traffic controllers are threatening a strike on the day we are supposed to fly home, and the postponed flights may be backed up for a day or two.

jwellbelove · 2025-06-02T17:30:13Z

I still have some concerns with copyright issues.

etl renames most of the class and variable names, except the name "inplace_function", which can be renamed too.

This seems to imply that this is not a 'cleanroom implementation', but actually contains a large proportion of copy/pasted code from another copyrighted project.

jwellbelove · 2025-06-02T17:33:05Z

include/etl/private/inplace_function_cpp11.h

+    static const vtable_t default_vtable;
+
+    const vtable_t* vtable_ptr{&default_vtable};
+    mutable char    obj[Capacity];


How can you ensure that this storage is aligned correctly for the type?

Can adopt etl::aligned_storage here. However, should we proceed with this PR or compare the implementation with the etl::function v2 ( https://etlcpp.slack.com/archives/C7U0HPFJA/p1742585235826159 ) first?

sorrowed · 2025-06-02T18:08:14Z

I still have some concerns with copyright issues.

etl renames most of the class and variable names, except the name "inplace_function", which can be renamed too.

This seems to imply that this is not a 'cleanroom implementation', but actually contains a large proportion of copy/pasted code from another copyrighted project.

I'm assuming a lot of this would be "inspired" by the sg14 implementation.

jiangyilism · 2025-06-16T08:30:14Z

I still have some concerns with copyright issues.

etl renames most of the class and variable names, except the name "inplace_function", which can be renamed too.

This seems to imply that this is not a 'cleanroom implementation', but actually contains a large proportion of copy/pasted code from another copyrighted project.

As discuss earlier, inplace_function can be renamed too.

rolandreichweinbmw · 2025-06-16T08:39:04Z

This implementation however somehow indicates "Copyright(c) 2025 BMW AG". Is that in itself even compatible with the ETL?

Copyright can be by any contributor. So it is "compatible" be default, and license is MIT.

Provide similar api to std::function. Store a callable inside the function object's member field in a type-erasure way. The member field, i.e. storage or buffer, has a compile-time fixed size. The size is specify either by the macro ETL_INPLACE_FUNCTION_DEFAULT_CAPACITY or a non-type template parameter.

jiangyilism · 2025-06-27T11:33:28Z

@jwellbelove inplace_function it is now renamed to owning_callable .
However, the idea is the same as SG14 inplace_function (boost license). So the implementation and the abi will still be similar.

About the aligned_storage adoption, does etl::owning_callable really need it? std::aligned_storage is deprecated in c++23 with reasons: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1413r2.pdf
The usage in SG14 ( https://github.com/WG21-SG14/SG14/blob/master/SG14/inplace_function.h#L370 ) does not seem necessary in my opinion. Since

The type-erasure behavior might make the calculated alignment non-optimal in some platforms
The alignment does not seem to be a performance bottleneck. Perhaps if we store many etl::owning_callable objects in an array then the alignment might make some difference. However, even same capacity-alignment owning_callable objects can, in run-time, hold totally different callees with different alignments. So I doubt that compile-time specified alignment value might not improve performance in all cases. While memory overhead is likely to occur in some cases.

sorrowed · 2025-06-28T20:33:07Z

@jwellbelove inplace_function it is now renamed to owning_callable . However, the idea is the same as SG14 inplace_function (boost license). So the implementation and the abi will still be similar.

I'm not sure why changing the name (which to me sounds less like what it does than inplace_function, which resembles for examples std::inplace_vector) changes anything copyright related?

jiangyilism · 2025-06-30T10:29:20Z

@jwellbelove inplace_function it is now renamed to owning_callable . However, the idea is the same as SG14 inplace_function (boost license). So the implementation and the abi will still be similar.

I'm not sure why changing the name (which to me sounds less like what it does than inplace_function, which resembles for examples std::inplace_vector) changes anything copyright related?

No matter how it deviate from sg14, there still share some similarity. All binaries look like a sequence of zeros and ones... If the current deviation is not enough, we need to decide what need to be met to avoid copyright issue.

sorrowed · 2025-06-30T17:59:39Z

@jwellbelove inplace_function it is now renamed to owning_callable . However, the idea is the same as SG14 inplace_function (boost license). So the implementation and the abi will still be similar.

I'm not sure why changing the name (which to me sounds less like what it does than inplace_function, which resembles for examples std::inplace_vector) changes anything copyright related?

No matter how it deviate from sg14, there still share some similarity. All binaries look like a sequence of zeros and ones... If the current deviation is not enough, we need to decide what need to be met to avoid copyright issue.

IANAL, but i don't think using SG14 as a source for your version is a problem as long as you honor the original license. The original Boost license of SG14 states that:

 * The copyright notices in the Software and this entire statement, including
 * the above license grant, this restriction and the following disclaimer,
 * must be included in all copies of the Software, in whole or in part, and
 * all derivative works of the Software

This means (IMHO, again IANAL) you can't change copyright to BMW AG (or any other entity) if the original copyright was claimed elsewhere and you can't change the license to MIT as the original license should be included.

The alternative is to create a cleanroom implementation but this can't be done by you as you already saw & used the SG14 version. Just changing some names in the implementation is not a solution at this point.

jiangyilism · 2025-07-10T07:11:51Z

If I understand the etl design correctly, we want to avoid introducing boost license.
@sorrowed Having seen sg14's implementation does not mean we can't implement something similar. But IANAL either.
@jwellbelove I think it is better to switch to your "function" v2 approach in another PR and avoid relating to this PR, which contains sg14 discussions, in any way.

jwellbelove · 2025-08-09T08:56:48Z

@coderabbitai review

coderabbitai · 2025-08-09T08:56:59Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai · 2025-08-09T08:57:02Z

Walkthrough

This update introduces a fixed-capacity, type-erased owning callable wrapper (etl::owning_callable) with comprehensive exception safety and copy/move semantics. Supporting traits (is_invocable_r) are added, and a thorough test suite validates the new functionality. Build configuration and error number macros are updated to integrate and track these additions.

Changes

Cohort / File(s)	Change Summary
Owning Callable Implementation `include/etl/owning_callable.h`, `include/etl/private/owning_callable_cpp11.h`	Introduced a new header and internal implementation for `etl::owning_callable`, a type-erased, fixed-capacity callable wrapper supporting copy/move semantics, exception safety, and runtime invocation checks.
Type Traits Enhancements `include/etl/type_traits.h`	Added `is_invocable_r` and `is_invocable_r_v` type traits, with custom SFINAE-based implementation for C++11, to detect callable invocability with return type checking.
Error Number Macro `include/etl/file_error_numbers.h`	Added macro definition for `ETL_OWNING_CALLABLE_FILE_ID` with value `"77"`.
Owning Callable Tests `test/test_owning_callable.cpp`	Added a comprehensive test suite covering construction, invocation, copy/move semantics, exception handling, and argument passing for `etl::owning_callable`.
Type Traits Tests `test/test_type_traits.cpp`	Added tests for the new `is_invocable_r` and `is_invocable_r_v` type traits, checking correct behaviour in various invocation scenarios.
Build System Update `test/CMakeLists.txt`	Included the new owning callable test file in the test suite build configuration.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant owning_callable
    participant Callable
    participant Exception

    User->>owning_callable: Construct with Callable
    owning_callable->>Callable: Store in internal buffer
    User->>owning_callable: Invoke operator()
    owning_callable->>Callable: Call with arguments
    Callable-->>owning_callable: Return result
    owning_callable-->>User: Return result

    User->>owning_callable: Invoke operator() on empty
    owning_callable->>Exception: Throw bad_owning_callable_call
    Exception-->>User: Exception thrown

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~40 minutes

Poem

In buffers deep, where callables sleep,
A wrapper now stands, its promise to keep.
With traits that discern if a function will fit,
And tests that ensure it won’t throw a fit.
New macros, new checks, all tidy and neat—
This codebase just got a clever new treat!
🚀✨

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ast-grep (0.38.6)

include/etl/type_traits.h

Note

🔌 MCP (Model Context Protocol) integration is now available in Early Access!

Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context.

✨ Finishing Touches

📝 Generate Docstrings

🧪 Generate unit tests

Create PR with unit tests
Post copyable unit tests in a comment

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Explain this complex logic.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai explain this code block.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and explain its main purpose.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR.
@coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
@coderabbitai generate unit tests to generate unit tests for this PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (7)

include/etl/type_traits.h (2)
2504-2528: Minor clean-up – specialise once for void

There’s no observable benefit in duplicating the specialisation for void and const void; the latter isn’t a valid return type and the extra template instantiation bloats compile times a little. Trimming to a single void specialisation keeps things tidy.

2487-2492: Alias template parameter name could mirror the standard

The alias template uses a generic typename... Types pack.
Adopting the standard’s (R, F, Args...) improves readability and grep-ability for anyone accustomed to std::is_invocable_r.
template <typename R, typename F, typename... Args>
using is_invocable_r = std::is_invocable_r<R, F, Args...>;
include/etl/owning_callable.h (1)
36-38: Consider an #else path for pre-C++11 builds

If a user compiles the ETL in C++03 mode, this header quietly becomes empty.
Adding an #else #error branch makes the limitation explicit and avoids silent ODR surprises.
#if ETL_USING_CPP11
  #include "private/owning_callable_cpp11.h"
#else
  #error "etl::owning_callable requires C++11 or later"
#endif
include/etl/private/owning_callable_cpp11.h (3)
25-28: Licence header contains stray “rhs” tokens

The MIT licence text has been mangled (…LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR rhsWISE…).
Please remove the “rhs” artefacts to avoid any legal ambiguity.

153-162: Copy-assignment performs an unnecessary extra copy

The parameter is passed by value and then moved from, so the callable is copied twice.
Consider the classical copy-and-swap:
-    owning_callable& operator=(owning_callable rhs) ETL_NOEXCEPT
+    owning_callable& operator=(owning_callable rhs) ETL_NOEXCEPT
     {
-      vtable_ptr->dtor_func(obj);
-      vtable_ptr = rhs.vtable_ptr;
-      rhs.vtable_ptr = &default_vtable;
-      vtable_ptr->move_func(obj, rhs.obj);
+      rhs.swap(*this);   // noexcept
       return *this;
     }
+
+    void swap(owning_callable& other) ETL_NOEXCEPT
+    {
+      etl::swap(vtable_ptr, other.vtable_ptr);
+      char tmp[Capacity];
+      vtable_ptr->move_func(tmp, obj);
+      other.vtable_ptr->move_func(obj, other.obj);
+      vtable_ptr->move_func(other.obj, tmp);
+    }
This removes the superfluous copy while keeping strong exception safety.

112-118: Cross-capacity copy only in constructor, not assignment

You allow owning_callable<CapA> → owning_callable<CapB> copy via the templated constructor but not via assignment. Providing a matching templated assignment operator would round-off the API.
test/test_owning_callable.cpp (1)
341-347: Beware silent narrowing in tests using floating literals

Calls such as f1(7.58) rely on implicit double → float conversion followed by float → int32_t truncation in the callable. The test passes today but conceals two silent truncations.

For clarity (and to avoid future surprises with -Wconversion):
CHECK_EQUAL(7, f1(7.58f));   // explicit float literal

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 293c7df and 41517ae.

📒 Files selected for processing (7)

include/etl/file_error_numbers.h (1 hunks)
include/etl/owning_callable.h (1 hunks)
include/etl/private/owning_callable_cpp11.h (1 hunks)
include/etl/type_traits.h (1 hunks)
test/CMakeLists.txt (1 hunks)
test/test_owning_callable.cpp (1 hunks)
test/test_type_traits.cpp (1 hunks)

🔇 Additional comments (4)

include/etl/file_error_numbers.h (1)

110-110: Looks good – ID sequence preserved

The new ETL_OWNING_CALLABLE_FILE_ID follows the existing sequential numbering scheme and doesn’t clash with any prior IDs. No action needed.
include/etl/type_traits.h (1)

2495-2501: Pass R by reference to avoid accidental copy requirements

accept(R) takes its parameter by value; that forces R to be copy-constructible.
std::is_invocable_r only requires convertibility, not copiability, so the current approach can incorrectly yield false for move-only return types.
-template <typename R>
-void accept(R);
+template <typename R>
+void accept(R&&) noexcept;
Using a forwarding reference removes the unintended copy constraint while preserving the convertibility check.

Likely an incorrect or invalid review comment.
test/CMakeLists.txt (1)

220-224: Thanks for wiring in the new tests

test_owning_callable.cpp is added in alphabetical order, so the list stays organised. Nice one.

test/test_type_traits.cpp (1)

1476-1498: Great coverage for is_invocable_r

The new test exercises both the trait and its _v shorthand across positive and negative cases – exactly what we need to guard against regressions.

coderabbitai · 2025-08-09T09:04:16Z

include/etl/private/owning_callable_cpp11.h

+
+    const vtable_t* vtable_ptr{&default_vtable};
+    mutable char obj[Capacity];
+


⚠️ Potential issue

Object buffer is not guaranteed to be suitably aligned

char obj[Capacity]; provides only 1-byte alignment, which is insufficient for many types (e.g. double, long long, SIMD types). Constructing such a type here is undefined behaviour on some targets.

A minimal fix:

- mutable char obj[Capacity]; + alignas(std::max_align_t) mutable char obj[Capacity];

or, if available in ETL:

- mutable char obj[Capacity]; + typename etl::aligned_storage_t<Capacity>::type obj;

📝 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.

Suggested change

const vtable_t* vtable_ptr{&default_vtable};

mutable char obj[Capacity];

const vtable_t* vtable_ptr{&default_vtable};

alignas(std::max_align_t) mutable char obj[Capacity];

🤖 Prompt for AI Agents

In include/etl/private/owning_callable_cpp11.h around lines 226 to 229, the object buffer obj is declared as char array which only guarantees 1-byte alignment, causing undefined behavior for types requiring stricter alignment. To fix this, replace the char array with a properly aligned storage type such as std::aligned_storage or an equivalent ETL type if available, ensuring the buffer has the correct size and alignment for any stored type.

jiangyilism commented Mar 6, 2025

View reviewed changes

rolandreichweinbmw approved these changes Mar 6, 2025

View reviewed changes

jwellbelove assigned jwellbelove and jiangyilism Mar 6, 2025

jwellbelove added this to Embedded Template Library Mar 6, 2025

github-project-automation bot moved this to In progress in Embedded Template Library Mar 6, 2025

jwellbelove reviewed Mar 6, 2025

View reviewed changes

jiangyilism force-pushed the yichiang/support_inplace_function branch 2 times, most recently from af095d7 to 064f21d Compare March 13, 2025 16:50

jiangyilism requested a review from jwellbelove March 13, 2025 16:51

jiangyilism force-pushed the yichiang/support_inplace_function branch from 064f21d to 2a9fb5d Compare March 17, 2025 09:21

BartolomeyKant reviewed Apr 1, 2025

View reviewed changes

denravonska reviewed Apr 3, 2025

View reviewed changes

jiangyilism force-pushed the yichiang/support_inplace_function branch from 2a9fb5d to a8c9a54 Compare April 7, 2025 06:09

jiangyilism force-pushed the yichiang/support_inplace_function branch from a8c9a54 to 2362d9d Compare April 7, 2025 06:15

BartolomeyKant approved these changes Apr 7, 2025

View reviewed changes

rolandreichweinbmw reviewed Apr 7, 2025

View reviewed changes

denravonska suggested changes Apr 7, 2025

View reviewed changes

jiangyilism force-pushed the yichiang/support_inplace_function branch from 2362d9d to d5726a9 Compare April 7, 2025 12:59

rolandreichweinbmw mentioned this pull request Apr 30, 2025

etl::delegate #1071

Closed

jwellbelove reviewed Jun 2, 2025

View reviewed changes

jiangyilism added 2 commits June 26, 2025 15:06

Support c++17 is_invocable_r

78dc812

jiangyilism force-pushed the yichiang/support_inplace_function branch from d5726a9 to 41517ae Compare June 27, 2025 11:15

coderabbitai bot reviewed Aug 9, 2025

View reviewed changes


		const vtable_t* vtable_ptr{&default_vtable};
		mutable char obj[Capacity];

Uh oh!

Support etl::inplace_function #1046

Are you sure you want to change the base?

Support etl::inplace_function #1046

Uh oh!

Conversation

jiangyilism commented Mar 6, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jwellbelove Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

denravonska commented Mar 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BartolomeyKant Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BartolomeyKant Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

denravonska commented Apr 3, 2025

Uh oh!

rolandreichweinbmw commented Apr 3, 2025

Uh oh!

rolandreichweinbmw commented Apr 3, 2025

Uh oh!

denravonska commented Apr 3, 2025

Uh oh!

BartolomeyKant commented Apr 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rolandreichweinbmw commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BartolomeyKant commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rolandreichweinbmw commented Apr 3, 2025

Uh oh!

denravonska commented Apr 3, 2025

Uh oh!

jiangyilism commented Apr 3, 2025

Uh oh!

denravonska commented Apr 3, 2025

Uh oh!

jwellbelove commented Apr 4, 2025

Uh oh!

jiangyilism commented Apr 7, 2025

Uh oh!

jiangyilism commented Mar 6, 2025 •

edited by coderabbitai bot

Loading

jwellbelove Mar 6, 2025 •

edited

Loading

BartolomeyKant Apr 1, 2025 •

edited

Loading

BartolomeyKant Apr 1, 2025 •

edited

Loading

rolandreichweinbmw commented Apr 3, 2025 •

edited

Loading

BartolomeyKant commented Apr 3, 2025 •

edited

Loading

denravonska commented Apr 7, 2025 •

edited

Loading

jwellbelove commented Apr 7, 2025 •

edited

Loading

rolandreichweinbmw commented Jun 16, 2025 •

edited

Loading