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

EvenQueue: fix template functions passing UserAllocatedEvent<...> as argument. #11391

Merged
merged 1 commit into from
Sep 3, 2019

Conversation

maciejbocianski
Copy link
Contributor

Fix EvenQueue template functions passing UserAllocatedEvent<...> as argument.

Description

Pull request type

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

Reviewers

Release Notes

Fix template functions passing UserAllocatedEvent<...> as argument.
Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why only ARMC5 failing?

@maciejbocianski
Copy link
Contributor Author

maciejbocianski commented Sep 2, 2019

Looks like ARMC6 has better template deduction algorithms? @kjbracey-arm

@ciarmcom ciarmcom requested review from a team September 2, 2019 15:00
@ciarmcom
Copy link
Member

ciarmcom commented Sep 2, 2019

@maciejbocianski, thank you for your changes.
@ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 2, 2019

CI started

@adbridge
Copy link
Contributor

adbridge commented Sep 2, 2019

@kjbracey-arm could you do a quick review ?

@mbed-ci
Copy link

mbed-ci commented Sep 2, 2019

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 2, 2019

CI needs restarting , internal fault

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 3, 2019

CI now restarted

template<typename... Args>
bool cancel(UserAllocatedEvent<Args...> *event)
template<typename F, typename A>
bool cancel(UserAllocatedEvent<F, A> *event)
Copy link
Contributor

@kjbracey kjbracey Sep 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Original code wasn't technically wrong, but I can see why ARMC5 might have got confused.

We're not actually instantiating anything here. UserAllocatedEvent takes two template arguments, and we're declaring something that could potentially give it variable.

There should only be a parameter mismatch if this was instantiated with Args... having anything other than 2 arguments. And that would never happen by function parameter deduction (there are no UserAllocatedEvents with anything other than 2 arguments to pass). It should only barf if explicitly called as cancel<int,int,int>(nullptr).

Presumably ARMC5 has some logic that crosschecks template arguments and parameters when a template is declared, and that's activating here, and thinking "variadic" doesn't match "2". That check may have been reasonable before variadic arguments. It catches this:

template <typename A, typename B>
class Foo;

template <typename A>
bool cancel(Foo<A> *event);

I don't actually know whether that is legal; and if it isn't, is a compiler is required to diagnose it if cancel is never instantiated?

Anyway, this is an example of the sort of ARMC5 glitch we're going to keep hitting. Its C++11 implementation is incomplete, and buggy. We'll have to keep working around it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, this is an example of the sort of ARMC5 glitch we're going to keep hitting. Its C++11 implementation is incomplete, and buggy. We'll have to keep working around it.

😭 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have section "11.13 C++11 supported features" from the ARM C 5.06 manual printed to 3 sheets of A4 and pinned to the side of my cubicle.

Recommended reading for anyone thinking of pushing their C++11 luck.

@mbed-ci
Copy link

mbed-ci commented Sep 3, 2019

Test run: SUCCESS

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants