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

Remove unused includes #243

Merged
merged 2 commits into from
May 15, 2024
Merged

Conversation

patkenneally
Copy link
Collaborator

@patkenneally patkenneally commented May 10, 2024

  • Tickets addressed: NA
  • Review: By commit
  • Merge strategy: Merge (no squash)

Description

The Basilisk code base is has many unused includes. This is wasted effort during compilation as the compiler often parses the include only to then never make use of that effort. This PR simply removes all unused includes.

Verification

CI runs successfully and by inspection the only changes should be removed includes.

Documentation

None new, none invalidated.

Future work

A tool like IWUYU should be added to prevent developers from merging code with unused includes.

Copy link
Collaborator

@joaogvcarneiro joaogvcarneiro left a comment

Choose a reason for hiding this comment

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

It looks good. I wonder why you also don't remove some unnecessary "return "statements when you clean up. Is it because you feel that's more of a refactor, not a cleanup? I am just trying to understand your thought process.

@patkenneally
Copy link
Collaborator Author

It looks good. I wonder why you also don't remove some unnecessary "return "statements when you clean up. Is it because you feel that's more of a refactor, not a cleanup? I am just trying to understand your thought process.

You make a good point. Pre-commit checks only for formatting changes. As you correctly note, removing a return statement is more of a code change. In time we will add a static analysis step to the PR process which will be able check for such things.

@patkenneally patkenneally force-pushed the feature/remove-unused-includes branch from d5ba5c1 to 8d13337 Compare May 15, 2024 16:08
@patkenneally patkenneally merged commit 080f8fa into develop May 15, 2024
3 checks passed
@patkenneally patkenneally deleted the feature/remove-unused-includes branch May 15, 2024 17:16
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.

3 participants