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

core: forward declare msg_t in thread.h #16477

Closed
wants to merge 1 commit into from

Conversation

kaspar030
Copy link
Contributor

Contribution description

Alternative to #16458.
This forward-declares msg_t in thread.h, and adapts thread.h, sched.h and msg.h accordingly.

Testing procedure

Issues/PRs references

@kaspar030 kaspar030 added Area: core Area: RIOT kernel. Handle PRs marked with this with care! Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels May 18, 2021
@kaspar030 kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 18, 2021
@@ -163,6 +162,11 @@ extern "C" {
*/
typedef void *(*thread_task_func_t)(void *arg);

/**
* @brief Forward declaration of msg_t
Copy link
Member

Choose a reason for hiding this comment

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

I think if you use @ref struct msg here, doxygen will correctly render it. All in all, it would be nice to have a link to the actual struct, since this is in a completely different header.

@@ -168,7 +168,7 @@
#include <stdint.h>
#include <stdbool.h>

#include "sched.h"
#include "thread.h"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#include "thread.h"
#include "thread.h" /* include forward declaration of `msg_t`. */

?

@maribu
Copy link
Member

maribu commented May 18, 2021

With this, thread.h and sched.h still depend on each other. As a result, it will remain impossible to use helpers like thread_getpid() or thread_getactive() from within sched.h.

The problem with the cross header forward declaration is that they mask the underlying problem of circular dependencies, rather than solving it.

@kaspar030
Copy link
Contributor Author

The problem with the cross header forward declaration is that they mask the underlying problem of circular dependencies, rather than solving it.

Maybe, but I don't think that's the issue here. thread and sched are not really distinct modules. This shows in e.g., thread_getpid() and thread_getactive() returning sched internal variables. Maybe it is time to just merge sched into thread.

@miri64
Copy link
Member

miri64 commented May 18, 2021

Maybe it is time to just merge sched into thread.

Shouldn't it rather be the other way around? A kernel can't have threads without a scheduler, but the other way around it works ;-).

@kaspar030
Copy link
Contributor Author

Shouldn't it rather be the other way around? A kernel can't have threads without a scheduler, but the other way around it works ;-).

well, maybe. most parts of the system and most apps will use the thread API, though. not much from sched other than defines.

@miri64
Copy link
Member

miri64 commented May 18, 2021

Then I think there should be a thread API, that is implemented by the sched module ;-).

@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 22, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@stale
Copy link

stale bot commented Mar 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Mar 2, 2022
@stale stale bot closed this Apr 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: core Area: RIOT kernel. Handle PRs marked with this with care! CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR State: stale State: The issue / PR has no activity for >185 days Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants