-
Notifications
You must be signed in to change notification settings - Fork 189
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
Minor cleanup of testing framework code #5622
Minor cleanup of testing framework code #5622
Conversation
@@ -9,6 +9,7 @@ spectre_target_sources( | |||
${LIBRARY} | |||
PRIVATE | |||
Abort.cpp | |||
Exit.cpp |
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.
Did you intend to actually make the dependence on Charm PRIVATE
in this file as the commit message promises? If so I think it would be good to also add a comment there why you made it private so it won't accidentally be made public again later.
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.
I did, but I couldn't get it to work because we still have some cyclic library dependencies. I'll remove the comment from the commit message
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.
Do you understand the cyclic dependency? How can making a dependency private introduce a cyclic dependency?
7ea610f
to
6893a31
Compare
@nilsvu any other comments or requested changes? |
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.
LGTM
Proposed changes
Small changes aimed at reducing code complexity in order to improve compile time and reduce code coupling.
Upgrade instructions
Code review checklist
make doc
to generate the documentation locally intoBUILD_DIR/docs/html
.Then open
index.html
.code review guide.
bugfix
ornew feature
if appropriate.Further comments