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

Always define q #2266

Merged
merged 4 commits into from
Sep 24, 2018
Merged

Always define q #2266

merged 4 commits into from
Sep 24, 2018

Conversation

pdebuyl
Copy link
Contributor

@pdebuyl pdebuyl commented Sep 14, 2018

Fixes #2248 (tentative)

Description of changes:

  • remove preprocessor barriers to the definition of q as a particle propertpy
  • goal: avoid failure when a feature "blindly" accesses q (here, for the MDA sample)

I file the PR now to get an idea of the CI result, as I could only test a limited number of configurations.

PR Checklist

  • Tests?
    • Interface
    • Core
  • Docs?

The aim is to avoid undefined access to q when ELECTROSTATICS is not
defined.
@fweik
Copy link
Contributor

fweik commented Sep 14, 2018

This probably means that a lot of ifdef's can be removed.

@@ -598,6 +598,10 @@ int set_particle_q(int part, double q) {
mpi_send_q(pnode, part, q);
return ES_OK;
}
#ifndef ELECTROSTATICS
const constexpr double ParticleProperties::q;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can drop the const here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, const is needed in C++11, constexpr only implies const in C++14 an newer.

Copy link
Member

@KaiSzuttor KaiSzuttor Sep 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pdebuyl
Copy link
Contributor Author

pdebuyl commented Sep 14, 2018

Should I hunt down the #ifdef ELECTROSTATICS that protect access to q?

@fweik
Copy link
Contributor

fweik commented Sep 14, 2018

@pdebuyl that would be helpful. There is at least one for which the performance impact needs to be checked (the one in the pair force calculation).

@pdebuyl
Copy link
Contributor Author

pdebuyl commented Sep 14, 2018

next week :-)

@codecov
Copy link

codecov bot commented Sep 24, 2018

Codecov Report

Merging #2266 into python will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           python   #2266   +/-   ##
======================================
  Coverage      71%     71%           
======================================
  Files         377     377           
  Lines       18941   18941           
======================================
  Hits        13580   13580           
  Misses       5361    5361
Impacted Files Coverage Δ
src/core/particle_data.hpp 100% <ø> (ø) ⬆️
src/core/particle_data.cpp 91% <ø> (ø) ⬆️
src/core/utils/serialization/List.hpp 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd3ea21...5966752. Read the comment docs.

@fweik fweik merged commit 97f7df4 into espressomd:python Sep 24, 2018
@pdebuyl pdebuyl deleted the always_define_q branch September 25, 2018 13:07
@RudolfWeeber RudolfWeeber added this to the Espresso 4.0.1 milestone Oct 15, 2018
RudolfWeeber pushed a commit to RudolfWeeber/espresso that referenced this pull request Oct 15, 2018
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