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

Match enet_tasklet_disconnect() declaration to definition #8698

Closed
wants to merge 9 commits into from

Conversation

elm3
Copy link

@elm3 elm3 commented Nov 9, 2018

Description

The declaration of enet_tasklet_disconnect() was declared in the header to return a void. In definition, it returned int8_t. This fix is just to correct the header to match the code.

Pull request type

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

The declaration of enet_tasklet_disconnect() was declared in the header to return a void.  In definition, it returned int8_t.  This fix is just to correct the header to match the code.
@kjbracey
Copy link
Contributor

kjbracey commented Nov 12, 2018

As you've now noted in the issue - the parameters also mismatch, so fix that too.

Also, it seems the error was able to arise because ethernet_tasklet.c failed to include "include/enet_tasklet.h"

Please fix that at the same time - the other tasklets have their own-header include before "include/mesh_system.h".

@kjbracey
Copy link
Contributor

The send_cb parameter should be true in this case.

That does raise the question of what happened to any calls that pass false. It appears such calls used to happen in various destructors, but they vanished in commit 27cced7. Intentionally, @SeppoTakalo ?

In the earlier commit, I fixed the return code.  In this fix, I added the arguments so that the signature exactly matches the function.
By including this header in the source, we are having the compiler keep the function and declaration the same.
Copy link
Contributor

@SeppoTakalo SeppoTakalo left a comment

Choose a reason for hiding this comment

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

Build failures now.

@kjbracey
Copy link
Contributor

As I said above, the one remaining call to this needs to pass true. The calls passing false from destructors no longer exist.

the enet_tasklet_disconnect() signature is now fixed to correctly require an argument (boolean).

This call (the only call) now calls the function with a boolean correctly.
I added the wrong boolean in this fixed call.
The signature said it returned uint8_t but in reality the function returned int8_t.
"mesh_connection_status_t" is not defined without the header.
Copy link
Contributor

@SeppoTakalo SeppoTakalo left a comment

Choose a reason for hiding this comment

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

Looks good, but please squash the commits together.

7 commits and 5 lines changed :D

The declaration of enet_tasklet_XXXX() functions was declared in the header file.  Some of the declarations didn't match the
definitions so those were fixed.  This let to some other issues, where the calls where not called cleanly.  All this was
corrected and is now building cleanly.
@cmonr
Copy link
Contributor

cmonr commented Nov 20, 2018

@elm3 Could you take a look at your last commit? Something doesn't look right with it.

Whenever you do a rebase, you end up force-pushing your branch, not generating a merge commit.

@cmonr
Copy link
Contributor

cmonr commented Nov 29, 2018

@elm3 ?

@cmonr
Copy link
Contributor

cmonr commented Dec 4, 2018

Closing since no response/activity has occurred in over two weeks.

Feel free to reopen once an update is available.

@cmonr cmonr closed this Dec 4, 2018
@cmonr cmonr removed the needs: work label Dec 4, 2018
deece added a commit to InfernoEmbedded/mbed-os that referenced this pull request Dec 14, 2018
Rework of ARMmbed#8698

Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
cmonr pushed a commit that referenced this pull request Dec 28, 2018
Rework of #8698

Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
@cmonr cmonr deleted the elm3-patch-1 branch January 11, 2019 05:16
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

5 participants