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

[new-core-alignment] service includes cleaned up #1358

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

rex-schilasky
Copy link
Contributor

@rex-schilasky rex-schilasky commented Feb 9, 2024

Description

Service header clean up, see see eclipse-ecal/ecal-core#15.

Based on changes by @FlorianReimold):

I tried to apply the IWYU pragma. I mainly focused on std headers, as those don't come with umbrella headers.

Cherry-pick to

  • None

Copy link
Contributor

@KerstinKeller KerstinKeller left a comment

Choose a reason for hiding this comment

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

👍
I think if we do cleanup like this, we should try to come up with a rule, e.g. first include your own header (e.g. client_session.cpp first include is client_session.h, then all standard headers, then all other thirdparty headers, and then all needed ecal headers?

Not 100% sure, but I would mix it like in server_manager.h 😄

@FlorianReimold
Copy link
Member

I usually include the class declaration .h (in cases of .cpp) first, then stl headers, then custom headers. But in this case it took a considerable amount of time to just go through the clang-tidy output and decide for each warning if I wanted to apply the fix or not (there were a couple thousand warnings).
And as the order of includes are not supposed to matter anyways, I just clicked the fix-it checkboxes. I think the manual labor of re-sorting this is not worth the time, I would rather merge this and then further improve the includes. I am certain that I missed a lot of includes that should be there and I didn't fix any unnecessary includes, yet (i.e. remove them). A further improvement would also be to mark umbrella headers accordingly with an IWYU export comment.

@KerstinKeller
Copy link
Contributor

I think, for eCAL6 we should think about getting rid of "umbrella headers". Yes it's nice that you can just include ecal.h and you get everything that you want, but is it necessary?

@rex-schilasky rex-schilasky merged commit ec1e344 into master Feb 12, 2024
15 checks passed
@rex-schilasky rex-schilasky deleted the new-core-alignment/service-includes-cleanup branch February 12, 2024 14:02
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.

3 participants