arobenko / embxx

embxx - Embedded C++ Library
GNU General Public License v3.0
262 stars 35 forks source link

StaticFunction const, nonconst #20

Open user706 opened 6 years ago

user706 commented 6 years ago

Hi,

related to #19 (and thus using code from branch develop) ... I'm just meddling with a similar library (ref), and came across an issue, that affects StaticFunction as well:

// ...

#include <iostream>

struct Func {
    void operator()() const {
        std::cout << "const   ";
    }

    void operator()()  {
        std::cout << "nonconst";
    }
};

int main()
{
    {
        Func func{};
        embxx::util::StaticFunction<void(), 32> f(func);
        f();
        std::cout << "\t should get ";
        func();
        std::cout << std::endl;
    }
    {
        const Func func{};
        embxx::util::StaticFunction<void(), 32> f(func);
        f();
        std::cout << "\t should get ";
        func();
        std::cout << std::endl;
    }
}

This prints

nonconst     should get nonconst
nonconst     should get const   

So with the mutable (ref)... the void operator()() const is not called in the end...

user706 commented 6 years ago

Ahhh... hang on!

std::function behaves the same. See here. So this specific issue is probably invalid and can be closed.

But I still wonder if it is somehow possible to get that nonconst const thing working...

Edit: Interesting reading here: "std::function does not enforce const-correctness: it allows you to store mutable callables"

arobenko commented 6 years ago

When resolving #19, I've taken a look at definition of [std::function::operator()](https://en.cppreference.com/w/cpp/utility/functional/function/operator()). It is defined to be "const", but at the same time it allows passed object to modify its state. It means that stored function object must be "mutable", there is no way around it. I tried to introduce both const and non-const operator() to StaticFunction, but could not get it working. I guess it's better to comply with std::function interface, than invent something new.

user706 commented 6 years ago

I'm experimenting with that other library, and have a quite raw state**, but with "interesting" behaviour regarding const non-const: No mutable and saner const behaviour... my_experiments

(** rhs of move operatings still needs fixup etc. )

PS... regarding the following line in your code, note that there are cases where it will fail: example:

#include <iostream>

class A
{
public:
    virtual ~A() = 0;
    virtual void a() = 0;
};

class B : A
{
public:
    void a() override
    {}
    alignas(16) int i;
};

int main()
{
    std::cout << alignof(A) << ' ' << alignof(B) << std::endl;
    return 0;
}

My recommendation for the alignment stuff is this (translated to your codebase) -- have TAlign as template parameter, and use std::max:

template <typename TSignature, std::size_t TSize = sizeof(void*) * 3, std::size_t TAlign = 0>
class StaticFunction;

//...

template <std::size_t TSize, std::size_t TAlign, typename TRet, typename... TArgs>
class StaticFunction<TRet (TArgs...), TSize, TAlign>
{
    static_assert(TAlign <= alignof(std::max_align_t), "please check your compiler to see if you can really align to greater than alignof(std::max_align_t)");

//...

    static constexpr std::size_t alignment = std::max(std::alignment_of<Invoker>::value, TAlign);

    typedef typename
        std::aligned_storage<
            StorageAreaSize,
            alignment
      >::type StorageType;

//Change:
// static_assert(alignof(Invoker) == alignof(InvokerBoundType),
//...
// to 
static_assert(alignof(InvokerBoundType) <= alignment, ...

This means... the user can widen the alignment beyond std::alignment_of<Invoker>::value, should he wish to do so.

I have a question regarding your code: why do you have a non-const param copy assignment operator (link)?

Thanks.