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

Misleading header file #512

Closed
wants to merge 2 commits into from

Conversation

theaverageguy
Copy link

Fixes #489

@wkerzendorf
Copy link
Member

@chvogl @yeganer @mreinecke can you guys look at this? I can't remember if that was a sensible fix.

@yeganer
Copy link
Contributor

yeganer commented Mar 7, 2016

@theaverageguy Thank you for the contribution.
@wkerzendorf The change is fine but there are two issues:

  • The 1st commit won't compile since there will be two declarations of line_search
  • After the 2nd commit there will be an import error because cmontecarlo1.h is deleted.

I suggest you remove the include "cmontecarlo1.h" from cmontecarlo.h and then combine all commits into one commit that covers the whole change with a litte explanation in the body of the commit message summarizing what and why it changed.

@mreineck
Copy link
Contributor

mreineck commented Mar 7, 2016

Also, you need to remove the
#include "cmontecarlo1.h"
from rpacket.h and add
#include "cmontecarlo1.h"
to rpacket.c.

@mreineck
Copy link
Contributor

mreineck commented Mar 7, 2016

Sorry, correction ....
of course you need to
#include "cmontecarlo.h"
from rpacket.c.

@yeganer
Copy link
Contributor

yeganer commented Mar 7, 2016

@mreineck thanks, I totally forgot that rpacket.c is missing this #include.

In general there needs to be some cleanup of header files. Headers should only #include ... stuff the header needs. Everything else goes into the corresponding source file.

@wkerzendorf I'm not sure if it is a good idea to include these changes within this PR or if we want a new PR for header cleanup.

@yeganer
Copy link
Contributor

yeganer commented Mar 31, 2016

#531 Fixed the corresponding issue. Closing this.

@yeganer yeganer closed this Mar 31, 2016
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.

4 participants