Skip to content

Fix include issue with Branch.hpp (#57) #58

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 1 commit into from
Feb 19, 2025
Merged

Fix include issue with Branch.hpp (#57) #58

merged 1 commit into from
Feb 19, 2025

Conversation

alexander-novo
Copy link
Collaborator

@alexander-novo alexander-novo commented Feb 17, 2025

Fixes #57

Also brought some adjacent includes in line with style guidelines

@alexander-novo
Copy link
Collaborator Author

This should fix the mis-compilation being experienced in the CI of #32

@pelesh pelesh added the bug Something isn't working label Feb 17, 2025
Copy link
Collaborator

@pelesh pelesh left a comment

Choose a reason for hiding this comment

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

Power flow branch header Branch.hpp is missing forward declaration to BranchData class. Adding the forward declaration should fix this issue.

The currently proposed solution introduces additional build dependencies that would be good to avoid.

@alexander-novo
Copy link
Collaborator Author

introduces additional build dependencies that would be good to avoid

@pelesh for future reference can you explain what you mean by this?

@pelesh
Copy link
Collaborator

pelesh commented Feb 17, 2025

introduces additional build dependencies that would be good to avoid

@pelesh for future reference can you explain what you mean by this?

In header files use forward declarations instead of including other headers whenever possible. That way, we have better control over what is included in each compilation unit.

@alexander-novo
Copy link
Collaborator Author

Changed to adding the forward declaration instead of including the definition. In the process I realized the phasor dynamics implementation already had this forward declaration, so we should be all good there.

@pelesh this forward declaration philosophy might be a good thing to put in the developer guidelines, since it seems like an easy thing to catch people unaware.

Copy link
Collaborator

@pelesh pelesh left a comment

Choose a reason for hiding this comment

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

Good to merge, thanks!

@pelesh pelesh merged commit 77f61bd into develop Feb 19, 2025
2 checks passed
@pelesh pelesh deleted the fix-includes branch March 14, 2025 03:36
pelesh pushed a commit that referenced this pull request Apr 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Branch.hpp doesn't include PowerSystemData.hpp even though it uses a type from that header
2 participants