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

avoid warning in gravity point postprocessor #2829

Merged
merged 2 commits into from Mar 1, 2019

Conversation

Projects
None yet
2 participants
@tjhei
Copy link
Member

commented Feb 25, 2019

I am getting warnings about un-initialized variables for the dim=2
instantiation that doesn't work anyways. Fix this.

avoid warning in gravity point postprocessor
I am getting warnings about un-initialized variables for the dim=2
instantiation that doesn't work anyways. Fix this.
@gassmoeller
Copy link
Contributor

left a comment

Yes, looks good. Please address my two minor points and merge yourself.

std::pair<std::string,std::string>
GravityPointValues<3>::execute (TableHandler &)
{
const int dim = 3;

This comment has been minimized.

Copy link
@gassmoeller

gassmoeller Feb 26, 2019

Contributor

const unsigned int

This comment has been minimized.

Copy link
@tjhei

tjhei Feb 26, 2019

Author Member

Interesting point. We normally use unsigned ints, but our template parameters are always

template <int dim>

It looks like we mostly use signed ints in ASPECT in this situation, but ~75% unsigned inside deal.II:

$ grep "const int dim" -r deal-git/source/ | wc -l
25
$ grep "const unsigned int dim" -r deal-git/source/ | wc -l
87

I will summon @bangerth for his opinion. :-)

@@ -52,10 +52,22 @@ namespace aspect
output_file_number (numbers::invalid_unsigned_int)
{}


This comment has been minimized.

Copy link
@gassmoeller

gassmoeller Feb 26, 2019

Contributor

I think we usually have 3 empty lines between functions (here and below).

@gassmoeller

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

Ok, lets not delay this PR further because of a not crucial discussion about int vs unsigned int. The content of the PR is definitely correct. We can think separately about unifying the templates if we ever want to. Thanks for the fix.

@gassmoeller gassmoeller merged commit 558c935 into geodynamics:master Mar 1, 2019

2 checks passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins.tjhei.info/pr-head This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.