Add cv-qualifiers to Callback and deprecate combinatorial explody functions #2496

geky · 2016-08-18T22:59:34Z

From this discussion #2395 and the related issue #2490

The combination of cv-qualifiers with callback overloads results in a large number of overloads to match all possible types that could be passed to the Callback class.

This isn't a big problem for void (*)(cv T *) function types, but for void (T::*)() cv types, the method in which cv-qualifiers are associated prevents encoding the cv-qualifiers in the T type, and requiring a large combination of overloads for all cv-qualifiers.

This pr involves several changes:

Adds full support for cv-qualifiers in Callback constructor
Provides convenience mbed::callback function to pass different arguments to Callback with infered type.
Deprecates existing convenience functions to avoid confusion if the user attempts to use a const or volatile object with the existing convenience functions.
Updates tests appropriately

Supported overloads:

callback(void (*f)(A...));
callback(const Callback<R(A...)> &);
callback(T *t,                void (*f)(T*, A...));
callback(const T *t,          void (*f)(const T*, A...));
callback(volatile T *t,       void (*f)(volatile T*, A...));
callback(const volatile T *t, void (*f)(const volatile T*, A...));
callback(T *t,                void (T::*f)(A...));
callback(const T *t,          void (T::*f)(A...) const);
callback(volatile T *t,       void (T::*f)(A...) volatile);
callback(const volatile T *t, void (T::*f)(A...) const volatile);

Should fix: #2490
Replaces: #2395 and #2495
cc @pan-, @0xc0170

Let me know any thoughts/suggestions on this solution you guys have.

pan- · 2016-08-19T10:49:31Z

That's a really nice change, I have few comments to make:

It would be interesting to make call and operator() const like in std::function, it allow user code to pass callback around with the guarantee that it will not be modified.
The operator= is missing.
Maybe it would be interesting to add external operators == and !=.
The call function might not be instantiated if R can't be constructed or casted from the literal 0 (see here).
Please provide relevant comments for every overload and template class specialization. It will help a lot with online documentation. Otherwise, user might get nothing if the overload is not documented or not useful comment like Templated function class.
It should be indicated that the file Callback.h is auto generated and should not be modified manually.

geky · 2016-08-19T16:56:54Z

+1, these are good points

Since a copy involves a simple copy of all the members, is the default operator= not sufficient?

Unfortunately the cast is for backwards compatibility, since at some point someone thought FunctionPointer f; f(); should be well defined. I can update it to value initialize R without breaking backwards compatibility, although that still assumes more about R than should be necessary.

pan- · 2016-08-19T17:32:30Z

Since a copy involves a simple copy of all the members, is the default operator= not sufficient?

Yes it is sufficient but in that case drop the copy construction operator (rule of 0 or rule of 3 (five in c++11)).

Unfortunately the cast is for backwards compatibility, since at some point someone thought FunctionPointer f; f();

The question is what we should do in that case ?
Instead of returning something totally invalid maybe it can be trapped into an error.

    R call() {
        if (!_thunk) {
            // never returns
            error("LOGIC_ERROR: Call to a not initialized callback!");
        }
        return _thunk(_obj, &_func);
    }

So FunctionPointer f; f(); is still valid, the user is notified in such case and there is template instantiation issues.

geky · 2016-08-19T17:43:52Z

The reason for explicitely defining the copy constructor was to make it clear that the Callback class could be copied safely. Tangentally do we expect this to work? Might need the assignment operator:

Callback<void()> cb;
void doit();
cb = doit;

(Still looking into where null callback is used)

geky · 2016-08-19T21:53:38Z

Well I can't seem to find where the null callback was relied on, I thought it was somewhere in our codebase. I went ahead and moved the null check into the deprecated FunctionPointer class to maintain that backwards compatibility. I tested a few things locally and we'll see if tests find any issues.

Interestingly enough, the operator= seems to work fine with C style function pointers at the moment, I think this is because it infers the cast before the assignment. I went ahead and added a test for that.

I'd prefer to keep the copy constructor since it clarifies the class can be safely copied and matches the overloads for attach and callback, though I can remove it if we really want it gone.

geky · 2016-08-19T22:20:18Z

Looks like it is a bit tricky, since several drivers are relying on uninitialized Callbacks being callable, however this is an easy fix.

pan- · 2016-08-22T08:50:30Z

Interestingly enough, the operator= seems to work fine with C style function pointers at the moment, I think this is because it infers the cast before the assignment. I went ahead and added a test for that.

Callback function pointer constructors are not tagged with explicit and therefore act as conversion constructors (more here). This is the expected behavior.

I'd prefer to keep the copy constructor since it clarifies the class can be safely copied and matches the overloads for attach and callback, though I can remove it if we really want it gone.

@0xc0170 @sg- Do we recommend any pattern like rule of three or rule of zero for C++ value types ?
What is the best approach here in term of readability and developer experience ?

Otherwise, +1 for the whole PR.

0xc0170 · 2016-08-23T09:55:42Z

@0xc0170 @sg- Do we recommend any pattern like rule of three or rule of zero for C++ value types ?
What is the best approach here in term of readability and developer experience ?

+1, this should be covered in the guidelines. I like a rule "all or nothing". We already provide "rule of three" or at least partially in our classes, we manage resources, thus inclined to rule "all", for us (c++03) three. If we agree, Callback should follow it.

The changes look good. People happy with jinja2 generated files?

sg- · 2016-08-23T13:30:38Z

I like a rule "all or nothing"

👍

The changes look good. People happy with jinja2 generated files?

nope

geky · 2016-08-23T14:45:25Z

@sg-, is there an alternative template engine we should be using? m4 perhaps?

Or do you suggest just not committing the template files?

sg- · 2016-08-23T15:30:20Z

Not committing the template files. If something needs to change that often to need auto-generation, I don't think its ready for master :)

geky · 2016-08-24T06:42:21Z

Copy contructor and jinja templates removed, let me know any other feedback.

0xc0170 · 2016-08-24T12:25:16Z

TESTS/mbed_drivers/callback/main.cpp

@@ -1,3 +1,6 @@
+// Note: This file was autogenerated from main.j2
+// j2 main.j2 > main.h


This is not true anymore?

Good catch!

0xc0170 · 2016-08-24T12:28:51Z

LGTM (one comment not valid anymore)

geky · 2016-08-24T15:59:07Z

Should be good to go 👍

geky · 2016-08-24T18:07:36Z

/morph test

mbed-bot · 2016-08-24T18:38:40Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 701

Build failed!

geky · 2016-08-24T23:25:23Z

Looks like all the template expansions in the test caused flash to exceed 64K : /

After talking with @bridadan (correct me if I'm wrong), will split the test into parts and move to TESTS/mbed_functional, which can house other non-hardware apis.

geky · 2016-08-25T15:51:47Z

/morph test

pan- · 2016-08-25T16:06:41Z

Looks like all the template expansions in the test caused flash to exceed 64K : /

At a latter stage maybe we can put all the data members in a non template base class and have function to set and reset them.

That way operator bool can be non template and reset and assignment can be non template.

It is also possible to factorize all operator== and operator!= instantiation, it is just a memcmp of this base class.

mbed-bot · 2016-08-25T17:19:16Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 715

All builds and test passed!

As the templated tests grew, the resulting binary exceeded a flash size of 64K. This caused the test to incorrectly fail on small devices. Moved and split into the following: TESTS/mbed_functional/callback TESTS/mbed_functional/callback_small TESTS/mbed_functional/callback_big TESTS/mbed_functional/functionpointer

geky · 2016-08-26T16:58:55Z

/morph test

mbed-bot · 2016-08-26T17:19:38Z

Result: ABORTED

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 728

Build Prep failed!

bridadan · 2016-08-26T17:19:46Z

@geky Sorry, making some CI changes. Please hold off on firing more PRs for the moment

geky · 2016-08-29T16:54:45Z

/morph test

mbed-bot · 2016-08-29T17:26:32Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 741

Test Prep failed!

bridadan · 2016-08-29T18:03:03Z

Looks like there was bug introduced with the latest version of setuptools, oh joy! I've deployed a workaround that should fix the test prep issue for now until they patch setuptools.

/morph test

mbed-bot · 2016-08-29T19:31:54Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 742

Test failed!

geky · 2016-08-29T23:35:08Z

/morph test

mbed-bot · 2016-08-30T01:11:26Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 745

Test failed!

bridadan · 2016-08-30T14:31:30Z

@geky Test failures look ok 👍 Just timing issues again from the CI machine being under load

geky · 2016-08-30T16:37:42Z

Ah ok, in that case this pr should be good to go

LGTM?

pan- · 2016-09-07T07:47:11Z

LGTM ?

teetak01 · 2016-09-10T07:42:14Z

Hi, this broke all mbed Client components. Please revert this.

https://jenkins-internal.mbed.com/view/mbed_Client/job/mbed-os-example-client_master/label=gcc,target=morpheus/114/console

yogpan01 · 2016-09-10T07:47:32Z

@sg- @geky @bridadan Why did you ignore #2653 because this PR has now broken the whole mbed-client builds ?
Please revert this PR ASAP or push the fix to resolve this. All our CI build jobs for mbed CLient are now broken and team is blocked because of this.
@jupe @marhil01 @tommikas FYI

jupe · 2016-09-10T08:55:43Z

we need also this #2656 to be merged asap because it could find this kind of issues.. but I'm wondering why any other CI job's didn't recognize this before it was too late..?

yogpan01 · 2016-09-10T08:58:50Z

Our nightly build job identified this issue , it has been failing since yesterday morning after this PR has been merged.

jupe · 2016-09-10T09:27:23Z

i think this kind of issues should be identified during PR verification jobs - before merging..

yogpan01 · 2016-09-10T09:30:15Z

mbed-os needs to add mbed-client build jobs to the PR verification queue.

sg- · 2016-09-10T11:16:10Z

@sg- @geky @bridadan Why did you ignore #2653 because this PR has now broken the whole mbed-client builds ?
Please revert this PR ASAP or push the fix to resolve this. All our CI build jobs for mbed CLient are now broken and team is blocked because of this.
@jupe @marhil01 @tommikas FYI

Will patch or revert. Wasnt aware of this issues as it wasnt linked to the pr until after the merge

sg- · 2016-09-10T11:16:40Z

i think this kind of issues should be identified during PR verification jobs - before merging..

There is no verification job on that catches this. They're missing from the jenkinsfile

sg- · 2016-09-10T11:17:11Z

we need also this #2656 to be merged asap because it could find this kind of issues.. but I'm wondering why any other CI job's didn't recognize this before it was too late..?

This has failing status so I cant merge 👎

geky mentioned this pull request Aug 18, 2016

Remove problematic templated overloads on callback-based functions #2395

Closed

This was referenced Aug 19, 2016

Add jinja template for Callback class #2495

Closed

Avoid combinatorial explosion in callback overloads #2490

Closed

geky force-pushed the callback-cv branch from e16f22b to d2deada Compare August 22, 2016 16:19

sg- added needs: review needs: CI labels Aug 23, 2016

0xc0170 reviewed Aug 24, 2016
View reviewed changes

geky force-pushed the callback-cv branch 2 times, most recently from 0c8ec15 to abd5430 Compare August 24, 2016 15:58

geky force-pushed the callback-cv branch from abd5430 to 9e6c42d Compare August 24, 2016 23:21

geky force-pushed the callback-cv branch from b4e3395 to 5c0f39f Compare August 26, 2016 14:54

sg- merged commit a6b2731 into ARMmbed:master Sep 8, 2016

sg- added release-version: 5.1.4 and removed ready for merge labels Sep 8, 2016

tommikas mentioned this pull request Sep 9, 2016

FEATURE_COMMON_PAL doesn't compile in master #2653

Closed

sg- removed the release-version: 5.1.4 label Sep 10, 2016

sg- added the release-version: 5.2.0 label Sep 26, 2016

geky mentioned this pull request Oct 24, 2016

master does not work with mbed OS 5.1.* when calling member function ARMmbed/mbed-events#25

Closed

		@@ -1,3 +1,6 @@
		// Note: This file was autogenerated from main.j2
		// j2 main.j2 > main.h

Add cv-qualifiers to Callback and deprecate combinatorial explody functions #2496

Add cv-qualifiers to Callback and deprecate combinatorial explody functions #2496

Uh oh!

Conversation

geky commented Aug 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pan- commented Aug 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geky commented Aug 19, 2016

Uh oh!

pan- commented Aug 19, 2016

Uh oh!

geky commented Aug 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geky commented Aug 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geky commented Aug 19, 2016

Uh oh!

pan- commented Aug 22, 2016

Uh oh!

0xc0170 commented Aug 23, 2016

Uh oh!

sg- commented Aug 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geky commented Aug 23, 2016

Uh oh!

sg- commented Aug 23, 2016

Uh oh!

geky commented Aug 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Aug 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geky commented Aug 24, 2016

Uh oh!

geky commented Aug 24, 2016

Uh oh!

mbed-bot commented Aug 24, 2016

Output

Uh oh!

geky commented Aug 24, 2016

Uh oh!

geky commented Aug 25, 2016

Uh oh!

pan- commented Aug 25, 2016

Uh oh!

mbed-bot commented Aug 25, 2016

Output

Uh oh!

geky commented Aug 26, 2016

Uh oh!

mbed-bot commented Aug 26, 2016

Output

Uh oh!

bridadan commented Aug 26, 2016

Uh oh!

geky commented Aug 29, 2016

Uh oh!

mbed-bot commented Aug 29, 2016

Output

Uh oh!

bridadan commented Aug 29, 2016

Uh oh!

mbed-bot commented Aug 29, 2016

Output

Uh oh!

geky commented Aug 29, 2016

Uh oh!

mbed-bot commented Aug 30, 2016

Output

Uh oh!

geky commented Aug 18, 2016 •

edited

Loading

pan- commented Aug 19, 2016 •

edited

Loading

geky commented Aug 19, 2016 •

edited

Loading

geky commented Aug 19, 2016 •

edited

Loading

sg- commented Aug 23, 2016 •

edited

Loading

0xc0170 commented Aug 24, 2016 •

edited

Loading