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

Change a few comments and make dust compile on crusher #14

Merged
merged 2 commits into from
Jan 4, 2023

Conversation

alwinm
Copy link

@alwinm alwinm commented Dec 21, 2022

Very light changes, hopefully useful.

@bcaddy
Copy link
Owner

bcaddy commented Jan 3, 2023

I'm confused by commit 9ea7336. What is the purpose of moving -Wall?

@alwinm
Copy link
Author

alwinm commented Jan 3, 2023

I'm confused by commit 9ea7336. What is the purpose of moving -Wall?

My understanding is that when -Wall is after Wno-unused-result it overrides it so that you still get unused result warnings. If you actually want Wno-unused-result to do anything it needs to be after Wall, since Wall turns on all warnings.

@bcaddy
Copy link
Owner

bcaddy commented Jan 4, 2023

Got it, thanks. Approving this now.

@bcaddy bcaddy merged commit 2656b5d into bcaddy:dev-mhd-integrator Jan 4, 2023
@helenarichie
Copy link

Thank you for doing this Alwin, I appreciate the help! I was wondering, though, why was this PR made on Bob's fork? Should I make them on the main cholla repo in the dev branch as well?

@bcaddy
Copy link
Owner

bcaddy commented Jan 4, 2023

There's some MHD fixes in there too. It probably should have been two PR's, one to my fork and one to Cholla in general.

@helenarichie
Copy link

Ah, gotcha. Well, just let me know if you'll be merging this work into the main cholla repo anytime soon, or if I should go ahead and add the changes there as well.

@bcaddy
Copy link
Owner

bcaddy commented Jan 4, 2023

The MHD PR should be merged in soon, hopefully by the end of the week. After that will be the Great Reformatting and then we should be back to operating as normal, though hopefully in a bit more agile mindset

@helenarichie
Copy link

Sounds great! Thanks!

@alwinm
Copy link
Author

alwinm commented Jan 4, 2023

Thank you for doing this Alwin, I appreciate the help! I was wondering, though, why was this PR made on Bob's fork? Should I make them on the main cholla repo in the dev branch as well?

I felt like piggybacking off of Bob's PR which will be merged into cholla soon. As Bob has pointed out, probably I should have just made 2 separate PRs but my laziness struck again

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