Skip to content

Fix header includes #1120

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

Merged
merged 4 commits into from
Mar 25, 2024
Merged

Conversation

HTRamsey
Copy link
Contributor

Hopefully will allow #1117 to pass
Ensures correct ordering of includes for everything.
Might be a bit pedantic, but having the core/optional include sections promotes the idea of add-on features rather than having everything intertwined. Similar to how cyclone is organized which I'm a big fan of.
Also, I removed the ipconfig checks for the headers because in my opinion it's cleaner to just guard the entire implementation with the ipconfig checks rather than the headers, which will have the same effect during builds but with less clutter. I'd suggest leaving them if the source files were organized in a module manner rather than a single root folder.

@HTRamsey HTRamsey requested a review from a team as a code owner March 20, 2024 18:12
Copy link
Member

@ActoryOu ActoryOu left a comment

Choose a reason for hiding this comment

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

Hi @HTRamsey,
Appreciate your contribution! I really like this change!
Left some comments.

BTW, I have tested this with #1117, and it works!

ActoryOu
ActoryOu previously approved these changes Mar 25, 2024
shubnil
shubnil previously approved these changes Mar 25, 2024
@ActoryOu ActoryOu dismissed stale reviews from shubnil and themself via e38441f March 25, 2024 05:44
@tony-josi-aws tony-josi-aws merged commit 207f971 into FreeRTOS:main Mar 25, 2024
10 checks passed
@HTRamsey HTRamsey deleted the dev-header-includes branch March 25, 2024 06:43
/* FreeRTOS includes. */
#include "FreeRTOS.h"
/* Global Includes & Definitions. */
#include "FreeRTOS_IP_Common.h"
Copy link
Member

Choose a reason for hiding this comment

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

adding FreeRTOS_IP_Common to bring in FreeRTOS.h and stdint.h also adds dependencies upon many other modules. This is a bad idea and encourages spaghetti code.

/* FreeRTOS includes. */
#include "FreeRTOS.h"
/* Global Includes & Definitions. */
#include "FreeRTOS_IP_Common.h"
Copy link
Member

Choose a reason for hiding this comment

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

adding FreeRTOS_IP_Common to bring in FreeRTOS.h also adds dependencies upon many other modules. This is a bad idea and encourages spaghetti code.

@n9wxu
Copy link
Member

n9wxu commented Mar 28, 2024

FreeRTOS_IP_Common.h is a bad idea. There is really nothing common about that header file and adding additional headers just creates dependencies where there should not be any.

I am reverting this PR. The common header should go away entirely with the other contents moved into new headers appropriate to the purpose. The OP compared with CycloneTCP as a a good example. I agree, the structure of CycloneTCP should be followed here with many small headers specific to the needed functionality.

n9wxu added a commit that referenced this pull request Mar 28, 2024
@HTRamsey
Copy link
Contributor Author

HTRamsey commented Mar 28, 2024

I agree that each header should only include what it needs, which the vast majority of them will at least need

#include "FreeRTOS.h"
#include "task.h"

#include "FreeRTOSIPConfig.h"
#include "FreeRTOSIPConfigDefaults.h"

and most of the typedefs in the headers should be avoided to allow forward declarations. Also many of the source files currently basically include everything. Right now most of the headers are missing the includes they need, which also caused issues with the freertos config values and freertos ipconfig values being available in the headers if they weren't included in certain orders. Unfortunately a restructuring like cyclone is a good deal of work.

@cobusve
Copy link
Member

cobusve commented Mar 28, 2024

There is probably a good case to consolidate the Config and defaults, if config has to ALWAYS be followed by defaults then it can be included in the config header file making the semantics of the dependency clear and resolving the risk of include order, but I agree we cannot make an aggregate header that includes all headers.

We are in the process of trying to restructure and we must make sure every step we take takes us closer to the target not the opposite direction.

At this point if anybody finds a header that uses something declared in a different header please fix that instance by including the header which it clearly depends on instead, that takes us closer to the goal.

We should also remove headers that are being included but are not being used wherever we see this.

@HTRamsey
Copy link
Contributor Author

HTRamsey commented Mar 28, 2024

@cobusve Since the config is a user provided file I guess we would have to just rely on the check here. While I clearly did it incorrectly, the purpose of this PR was to address the issue here where the problem was that FreeRTOS.h & task.h were needed to be included before FreeRTOSIPConfig.h & FreeRTOSIPConfigDefaults.h for each header, which wasn't the case for FreeRTOS_DNS.h at least. An alternative is to just not do any compile time config checks. Then you wouldn't need FreeRTOS.h & task.h for the config files. Or add more ifndef checks in the headers to make sure the includes are in the correct order.

AniruddhaKanhere pushed a commit that referenced this pull request Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants