[api] Fix handling of const objects in Callback class

[geky]

Before, the following results in a compilation error:

const struct Object *obj;
void obj_doit(const Object *obj);

Callback<void()> cb(obj, obj_doit);

This is especially noticable when migrating from the old Thread constructor, which previously required const.

Fixed by adding an additional const_cast:

void *_obj = const_cast<void*>(static_cast<const void*>(obj));

Could alternatively be fixed with C cast, not idiomatic C++ but cleaner:

void *_obj = (void*)obj;

cc @bridadan, @c1728p9

[geky]

@mbed-bot: TEST

HOST_OSES=windows
BUILD_TOOLCHAINS=GCC_ARM,ARM,IAR
TARGETS=K64F,LPC1768

[mbed-bot]

[Build 646]
SUCCESS: Building succeeded and tests were run! Be sure to check the test results

[pan-]

This is just straight undefined behavior and therefore illegal c++.

With this hack, it also totally mess with the overload resolution rules and failed at compilation time or worse execute the wrong function.

struct Foo { 

    int value() const { 
        return _ value;
    }

    void value(int new_value) { 
        _value = value;
    }

private:
   int _value;
};

const Foo foo;

// this callback should wrap int Foo::value() const
// instead the compiler will try to wrap void Foo::value(int)
// because the const has been cast away
Callback<int()> cb(&foo, Foo::value);

The solution is simple: instead of wrapping everything in

    struct _class;
    union {
        void (*_staticfunc)();
        void (*_boundfunc)(_class *);
        void (_class::*_methodfunc)();
    } _func;

    void *_obj;

    // Thunk registered on attach to dispatch calls
    R (*_thunk)(void*, void*, A0, A1, A2, A3, A4); 

And loose all type information when it is still needed, encapsulate everything into a generic function object which handle the call and preserve all type information. Store this object in an "byte storage" in the callback class and generate a function to call the function object, the equivalent of your thunk.

By using this approach, you can handle every kind of CV qualifier, not just const.

In the process you can also make the function call and the call operator const it is the same approach that is used by every serious function abstraction implementation.

[geky]

This is just straight undefined behavior and therefore illegal c++.

Which part is undefined behavior? Casting away constness is perfectly valid as long as the no write operation through the pointer (standard, see section 5.2.11).

With this hack, it also totally mess with the overload resolution rules and failed at compilation time or worse execute the wrong function.

This changes no function prototypes and can only impacts overload resolution where templates fail to expand. Your example fails to compile for reasons unrelated to this pr since Callback (and FunctionPointer before it) weren't designed to handle cv qualifiers.

Store this object in an "byte storage" in the callback class and generate a function to call the function object, the equivalent of your thunk.

Correct me if I'm wrong, but is the only difference between your suggestion and the current implementation the use of a byte array over a void * to store the _obj? Wouldn't you still need to lose the type information to store it in the byte array? The current implementation at least takes care of size/alignment concerns for the pointer.

By using this approach, you can handle every kind of CV qualifier, not just const.

You're right, this pr doesn't fix all of the cv qualifiers. I can look into adding this support through more specialized overloads. I may just be missing the bigger picture, but I'm not sure how the byte array with cv qualifiers more than void * casts.

In the process you can also make the function call and the call operator const

This is a good idea.

I was hoping to avoid expanding the Callback class too much, but it sounds like it may be necessary to properly handle cv qualifiers.

[pan-]

@geky

Which part is undefined behavior? Casting away constness is perfectly valid as long as the no write operation through the pointer (standard, see section 5.2.11).

This changes no function prototypes and can only impacts overload resolution where templates fail to expand. Your example fails to compile for reasons unrelated to this pr since Callback (and FunctionPointer before it) weren't designed to handle cv qualifiers.

I was under the impression that for member functions, the function pointer was converted to the non const version and I was wrong, it just generate a compilation error.
In that case there is no reasons to keep the const_cast for Callback with member function constructors (like here ).
And without them I would support that PR.

Correct me if I'm wrong, but is the only difference between your suggestion and the current implementation the use of a byte array over a void * to store the _obj? Wouldn't you still need to lose the type information to store it in the byte array? The current implementation at least takes care of size/alignment concerns for the pointer.

The important operation of a function call is the function call.
If this function call is generated from a function like object then all types information are still available when the function like object is generated. Only the call is type erased (easy) and the object is hidden into the storage.
It would be possible to do it with the current implementation but it would require:

• const
• volatile
• const volatile
  Overloads for each constructors and attach function (function and member function).

With a function like object as intermediate wrapper I think it is easy to generate the right code without tricky casts and it can greatly reduce the number of overloads needed.

It would be interesting to investigate how a the second parameter of the constructor and the attach function can be replaced by a template parameter type template<typename T, typename F>.
In that case, F will map automatically to the overload needed by T if their is no conflict.

Yes, the storage should be properly aligned.

[geky]

In that case there is no reasons to keep the const_cast for Callback with member function constructors

Good point, at the very least what I'm trying to do there seems completely broken because of member function pointer shenanigans. I'll remove them next chance I get.

The important operation of a function call is the function call.
If this function call is generated from a function like object then all types information are still available when the function like object is generated. Only the call is type erased (easy) and the object is hidden into the storage.

It'd be interesting to see what this would look like. I'd just get worried if it became more obscure how the class operated, but maybe it wouldn't be that bad.

It would be interesting to investigate how a the second parameter of the constructor and the attach function can be replaced by a template parameter type template<typename T, typename F>.
In that case, F will map automatically to the overload needed by T if their is no conflict.

That would be very cool, hopefully errors don't get too bad. It looks like gcc's implementation just has an overload for each combination of cv:
https://gcc.gnu.org/onlinedocs/gcc-4.7.0/libstdc++/api/a01173_source.html#l01150

[0xc0170]

Good point, at the very least what I'm trying to do there seems completely broken because of member function pointer shenanigans. I'll remove them next chance I get.

Separate PR for this?

Ready for merging? to fix the above use case (#2190 (comment))

[geky]

@0xc0170, sorry, I meant I would remove it from this pr once I had access to a computer. Give me just a bit.

[geky]

@mbed-bot: TEST

HOST_OSES=windows
BUILD_TOOLCHAINS=GCC_ARM,ARM,IAR
TARGETS=K64F,LPC1768

[geky]

I updated the pr to not change the constructors for member functions.

note: I changed the casts to C-style casts to completely strip out cv-qualifiers (without access to std::remove_cv). It's a bit of a hack that takes care of the current issue, although still type-safe since cv-qualifiers are carried over in the T type.

@pan- is right that we should adopt a set of overloaded constructors in the future.

[mbed-bot]

[Build 657]
SUCCESS: Building succeeded and tests were run! Be sure to check the test results