Various fixes based on static analysis. (#857)

* Fix "use after move" in DiscreteDemography._reset_state, which is part of the back-end for pickling this type. (clang-tidy) * Use const where possible. (cppcheck) * Fix redundant checks in if statements. (cppcheck) * Pass strings by const reference. (cppcheck) * Update skipped test comment to refer to issue #777
molpopgen · Nov 23, 2021 · 2f4d918 · 2f4d918
1 parent bdb4979
commit 2f4d918
Show file tree

Hide file tree

Showing 12 changed files with 27 additions and 21 deletions.
diff --git a/doc/misc/changelog.md b/doc/misc/changelog.md
@@ -3,6 +3,14 @@
 Major changes are listed below.  Each release likely contains fiddling with back-end code,
 updates to latest `fwdpp` version, etc.
 
+## 0.17.0a2
+
+Bug fixes:
+
+* Fixed a "use after move" error in C++ code used to pickle {class}`fwdpy11.DiscreteDemography`.
+  It looks like this bug was introduced back in {pr}`791`, which was part of the 0.16.0 release.
+  PR {pr}`857`
+
 ## 0.17.0a1
 
 Changes to `fwdpy11.conditional_models`:

diff --git a/fwdpy11/headers/fwdpy11/evolvets/recorders.hpp b/fwdpy11/headers/fwdpy11/evolvets/recorders.hpp
@@ -36,7 +36,7 @@ namespace fwdpy11
      */
     {
         inline void
-        operator()(const DiploidPopulation&, SampleRecorder&) const
+        operator()(const DiploidPopulation&, SampleRecorder&)
         {
         }
     };

diff --git a/fwdpy11/src/discrete_demography/DiscreteDemography.cc b/fwdpy11/src/discrete_demography/DiscreteDemography.cc
@@ -175,9 +175,8 @@ init_DiscreteDemography(py::module& m)
                 d["selfing_rates"].cast<ddemog::selfing_rates_vector::value_type>());
 
             ddemog::deme_properties sizes_rates(
-                std::move(current_deme_sizes), std::move(next_deme_sizes),
-                std::move(growth_rates_onset_times), std::move(growth_initial_sizes),
-                std::move(growth_rates), std::move(selfing_rates));
+                current_deme_sizes, next_deme_sizes, growth_rates_onset_times,
+                growth_initial_sizes, growth_rates, selfing_rates);
 
             ddemog::MigrationMatrix M{};
             auto t = d["migmatrix"].cast<py::tuple>();

diff --git a/fwdpy11/src/evolve_population/cleanup_metadata.cc b/fwdpy11/src/evolve_population/cleanup_metadata.cc
@@ -10,7 +10,7 @@ cleanup_metadata(const fwdpp::ts::std_table_collection& tables,
 {
     metadata.erase(
         std::remove_if(begin(metadata), end(metadata),
-                       [generation, &tables](fwdpy11::DiploidMetadata& md) {
+                       [generation, &tables](const fwdpy11::DiploidMetadata& md) {
                            return tables.nodes[md.nodes[0]].time
                                       == generation
                                   || tables.nodes[md.nodes[1]].time

diff --git a/fwdpy11/src/evolve_population/remove_extinct_mutations.cc b/fwdpy11/src/evolve_population/remove_extinct_mutations.cc
@@ -67,7 +67,7 @@ remove_extinct_mutations(fwdpy11::Population& pop)
                 }
         }
     pop.mutations.erase(std::remove_if(begin(pop.mutations), end(pop.mutations),
-                                       [](fwdpy11::Mutation& m) {
+                                       [](const fwdpy11::Mutation& m) {
                                            return m.pos
                                                   == std::numeric_limits<double>::max();
                                        }),

diff --git a/fwdpy11/src/evolve_population/track_mutation_counts.cc b/fwdpy11/src/evolve_population/track_mutation_counts.cc
@@ -13,7 +13,7 @@ track_mutation_counts(fwdpy11::Population &pop, const bool simplified,
             throw std::runtime_error(
                 "track_mutation_counts: count vector size mismatch");
         }
-    if (!simplified || (simplified && suppress_edge_table_indexing))
+    if (!simplified || suppress_edge_table_indexing)
         {
             fwdpp::fwdpp_internal::process_haploid_genomes(
                 pop.haploid_genomes, pop.mutations, pop.mcounts);

diff --git a/fwdpy11/src/functions/add_mutation.cc b/fwdpy11/src/functions/add_mutation.cc
@@ -174,14 +174,13 @@ namespace
                                                 s = si();
                                             }
                                         if (deme < 0
-                                            || (deme >= 0
-                                                && std::all_of(
-                                                    begin(descendants), end(descendants),
-                                                    [&pop,
-                                                     deme](fwdpp::ts::table_index_t i) {
-                                                        return pop.tables->nodes[i].deme
-                                                               == deme;
-                                                    })))
+                                            || std::all_of(
+                                                begin(descendants), end(descendants),
+                                                [&pop,
+                                                 deme](fwdpp::ts::table_index_t i) {
+                                                    return pop.tables->nodes[i].deme
+                                                           == deme;
+                                                }))
                                             {
                                                 if (node_has_valid_time(
                                                         pop.tables->nodes, n,

diff --git a/fwdpy11/src/fwdpp_types/DataMatrix.cc b/fwdpy11/src/fwdpp_types/DataMatrix.cc
@@ -69,7 +69,7 @@ init_data_matrix(py::module &m)
 
     py::class_<fwdpp::data_matrix, std::shared_ptr<fwdpp::data_matrix>>(
         m, "ll_DataMatrix")
-        .def(py::init([](std::shared_ptr<fwdpp::data_matrix> &p) {
+        .def(py::init([](const std::shared_ptr<fwdpp::data_matrix> &p) {
             if (p == nullptr)
                 {
                     throw std::invalid_argument("input pointer is nullptr");

diff --git a/fwdpy11/src/fwdpp_types/TableCollection.cc b/fwdpy11/src/fwdpp_types/TableCollection.cc
@@ -32,7 +32,7 @@ init_ts_TableCollection(py::module& m)
     // TODO: allow access to the "right" member functions
     py::class_<fwdpp::ts::std_table_collection,
                std::shared_ptr<fwdpp::ts::std_table_collection>>(m, "ll_TableCollection")
-        .def(py::init([](std::shared_ptr<fwdpp::ts::std_table_collection>& self) {
+        .def(py::init([](const std::shared_ptr<fwdpp::ts::std_table_collection>& self) {
             if (self == nullptr)
                 {
                     throw std::invalid_argument("input tables cannot be nullptr");

diff --git a/fwdpy11/src/fwdpy11_types/DiploidPopulation.cc b/fwdpy11/src/fwdpy11_types/DiploidPopulation.cc
@@ -121,7 +121,7 @@ init_DiploidPopulation(py::module& m)
                 return pop;
             }))
         .def("_dump_to_file",
-             [](const fwdpy11::DiploidPopulation& pop, const std::string filename) {
+             [](const fwdpy11::DiploidPopulation& pop, const std::string & filename) {
                  std::ofstream out(filename.c_str(), std::ios_base::binary);
                  if (!out)
                      {
@@ -132,7 +132,7 @@ init_DiploidPopulation(py::module& m)
              })
         .def_static(
             "_load_from_file",
-            [](const std::string filename) {
+            [](const std::string & filename) {
                 std::ifstream in(filename.c_str(), std::ios_base::binary);
                 if (!in)
                     {

diff --git a/fwdpy11/src/genetic_values/PyDiploidGeneticValue.cc b/fwdpy11/src/genetic_values/PyDiploidGeneticValue.cc
@@ -95,7 +95,7 @@ strict_additive_effects(const fwdpy11::DiploidPopulation& pop,
                         const fwdpy11::DiploidMetadata& individual)
 {
     double g = 0.0;
-    auto& diploid = pop.diploids[individual.label];
+    const auto& diploid = pop.diploids[individual.label];
     for (auto k : pop.haploid_genomes[diploid.first].smutations)
         {
             g += pop.mutations[k].s;

diff --git a/tests/test_discrete_demography_with_tree_sequences.py b/tests/test_discrete_demography_with_tree_sequences.py
@@ -810,7 +810,7 @@ def test_evolve_in_two_steps_restart_with_two_demes(self):
         self.assertEqual(deme_sizes[1][1], self.N1t)
 
     @unittest.skip(
-        reason="Fixing issue 775 means that this no longer works because pickling DiscreteDemography does not fully round-trip the internal model state"
+        reason="Fixing issue 775 means that this no longer works because pickling DiscreteDemography does not fully round-trip the internal model state. See issue 777"
     )
     def test_evolve_in_two_steps_restart_with_two_demes_and_pickle(self):
         """