Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce Callback.h using C++11 #10885

Merged
merged 1 commit into from Jul 9, 2019

Conversation

@kjbracey-arm
Copy link
Contributor

commented Jun 24, 2019

Description

The bulk of Callback.h was auto-generated as 6 specialisations, handling zero to five arguments.

This can now be handled without specialisation using C++11 variadic templates, reducing the file from 4,900 lines to 900 lines.

This should reduce compilation time, and offset potential increases from use of <type_traits> or a local mbed_cxxsupport.h equivalent.

Several other improvents to Callback are possible and/or desirable with C++11, such as the ability to store lambdas, but this commit is purely the variadic simplification.

Pull request type

[ ] Fix
[X] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@pan-, @bulislaw, @geky

The bulk of Callback.h was auto-generated as 6 specialisations, handling
zero to five arguments.

This can now be handled without specialisation using C++11 variadic
templates, reducing the file from 4,900 lines to 900 lines.

This should reduce compilation time, and offset potential increases from
use of `<type_traits>` or a local `mbed_cxxsupport.h` equivalent.

Several other improvents to `Callback` are possible and/or desirable
with C++11, such as the ability to store lambdas, but this commit is
purely the variadic simplification.
@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

The header file is actually somewhat understandable after this - it is complex, but the previous version further obfuscated it by just repeating everything 6 times, meaning it was rather hard to visualise the structure.

C++11 could possibly simplify it further using type traits to cut out a bunch of the const/volatile overloading repetition.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

Could get it down to 500 lines if all the deprecated stuff was dropped...

@pan-
pan- approved these changes Jun 24, 2019
Copy link
Member

left a comment

Look goods to me.
I've been working on a new implementation yesterday, that reduces the code size and simplify most of the (unnecessary) overloads and add other goodies. I will submit it once this first simplification goes in.

@ciarmcom ciarmcom requested review from bulislaw, geky, pan- and ARMmbed/mbed-os-maintainers Jun 24, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

@evedon
evedon approved these changes Jun 24, 2019
Copy link
Contributor

left a comment

That's great, thanks Kevin!
I was discussing with the team this optimisation just a few days ago as this file was really huge! Did not know that you had plan to work on it :)

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

I've been working on a new implementation yesterday, that reduces the code size and simplify most of the (unnecessary) overloads and add other goodies. I will submit it once this first simplification goes in.

Cool. I'll hold off on my temptation to fiddle further then, and wait to review yours.

@pan-

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

@evedon events/Event.h would also benefit from such simplification.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

It would make sense to do the same thing to Event.h at the same time. I can do that now, if no-one else has already done it.

@evedon

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

It would make sense to do the same thing to Event.h at the same time. I can do that now, if no-one else has already done it.

We have not worked on it so feel free.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

Hmm, Event.h and EventQueue.h are much more complex, as they don't just have to forward variadic arguments, they have to store them (in the EventQueue::context<x><y> structure). Nowhere near as straightforward - solutions involve a bunch of extra standard library machinery like std::tuple, which we don't have for ARM C 5.

@evedon

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

In that case, it is not worth doing it as part of this PR.

@geky
geky approved these changes Jun 25, 2019
Copy link
Member

left a comment

Glad to see this come in 👍

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

Glad to see this come in 👍

No doubt this was what you were yearning to write in the first place. The hard part was generating the C++98 version!

Hmm, Event.h and EventQueue.h are much more complex,

Actually, they're not so bad. I think I can remove 95% of them - it's just the innermost struct context that will remain specialized. I'll make a separate PR for them.

@artokin

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

@kjbracey-arm , what is status of this PR?

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

I think it's ready. Got a pile of design changes to follow, but this one's fine.

@artokin

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Jul 8, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@artokin artokin removed the needs: CI label Jul 9, 2019
@artokin artokin merged commit 28eb39c into ARMmbed:master Jul 9, 2019
26 checks passed
26 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8509 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8448B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
@artokin artokin removed the ready for merge label Jul 9, 2019
@kjbracey-arm kjbracey-arm deleted the kjbracey-arm:callback_variadic branch Jul 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.