-
Notifications
You must be signed in to change notification settings - Fork 5
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
Conversation
This should fix the mis-compilation being experienced in the CI of #32 |
There was a problem hiding this 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.
@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. |
82ff81c
to
e95dea6
Compare
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. |
There was a problem hiding this 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!
Fixes #57
Also brought some adjacent includes in line with style guidelines