Merge pull request #512 from m-a-d-n-e-s-s/evaleev/feature/detect-wor…

…ld-object-premature-deletion

harden World unique object handling

.github/workflows/cmake.yml

@@ -33,6 +33,7 @@ jobs:
       CTEST_OUTPUT_ON_FAILURE : 1
       BUILD_CONFIG : >
         -G Ninja
+        -DCMAKE_BUILD_TYPE=${{ matrix.build_type }}
         -DCMAKE_INSTALL_PREFIX=${{github.workspace}}/install
         -DCMAKE_PREFIX_PATH=/usr/local/opt/bison
         -DBUILD_SHARED_LIBS=OFF

doc/libraries/parallel_runtime/futures.dox

@@ -77,7 +77,7 @@ second element of \c v you'll get a runtime exception.
 The fix (other than using arrays) is to initialize STL vectors and
 other containers from the special element returned by
 \c Future<T>::default_initializer(), which if passed into the copy
-constructor will cause it to behave just like the default contructor.
+constructor will cause it to behave just like the default constructor.
 Thus, the following code is what you actually need to use an STL
 vector of futures
 \code

doc/tutorial/test_runtime.cpp

@@ -1,4 +1,5 @@
 #include <madness/world/worldgop.h>
+#include <cassert>
 
 int task (int i) { return i + 1; }
 
@@ -9,18 +10,14 @@ int main(int argc, char* argv[]) {
   const auto me = world.rank();
   const auto nproc = world.size();
 
-  Future<int> f;
   if (me == 0) {
-    f = world.taskq.add(&task, 0);
-    for (auto t = 1; t != 1000; ++t) {
+    for (auto t = 0; t != 1000; ++t) {
       const auto proc = t % nproc;
-      f = world.taskq.add(
-          proc, &task, f);
+      auto f = world.taskq.add(proc, &task, t);
+      assert(f.get() == t+1);
     }
   }
   world.gop.fence();
-  if (f.get() != 1000)
-    abort();
 
   finalize();
   return 0;

src/apps/dirac/fcwf.cc

@@ -77,7 +77,7 @@ unsigned int Fcwf::size() const {
      return m_psi.size();
 }
 
-//copy contructor defaults to deep copy
+//copy constructor defaults to deep copy
 //if this ever changes, you will need to change the copy() function, as it calls this
 Fcwf::Fcwf(const Fcwf& phi){
      MADNESS_ASSERT(m_psi.size() == 0);

src/examples/hatom_sf_dirac.cc

@@ -294,7 +294,7 @@ void run(World& world) {
         gradV[axis] = real_factory_3d(world).functor(p).truncate_mode(0);
     }
 
-    
+
     coord_3d lo = {0.0,0.0,-L}, hi = {0.0,0.0,L};
     const int npt = 1001;
 

src/madness/chem/CCStructures.h

@@ -138,7 +138,7 @@ struct CCMessenger {
 
 /// Timer Structure
 struct CCTimer {
-    /// TDA_TIMER contructor
+    /// TDA_TIMER constructor
     /// @param[in] world the world
     /// @param[in] msg	a string that contains the desired printout when info function is called
     CCTimer(World& world, std::string msg) : world(world), start_wall(wall_time()), start_cpu(cpu_time()),

src/madness/mra/convolution1d.h

@@ -862,12 +862,13 @@ namespace madness {
 
             iterator it = map.find(key);
             if (it == map.end()) {
-                map.insert(datumT(key, std::make_shared< GaussianConvolution1D<Q> >(k,
+                [[maybe_unused]] auto&& [tmpit, inserted] = map.insert(datumT(key, std::make_shared< GaussianConvolution1D<Q> >(k,
                                                                                     Q(sqrt(expnt/constants::pi)),
                                                                                     expnt,
                                                                                     m,
                                                                                     periodic
                                                                                     )));
+                MADNESS_ASSERT(inserted);
                 it = map.find(key);
                 //printf("conv1d: making  %d %.8e\n",k,expnt);
             }

src/madness/mra/funcimpl.h

@@ -5377,7 +5377,7 @@ namespace madness {
                 const keyT& key = it->first;
                 const FunctionNode<T,NDIM>& node = it->second;
                 if (node.has_coeff()) {
-                    map->insert(acc,key);
+                    [[maybe_unused]] auto inserted = map->insert(acc,key);
                     acc->second.push_back(std::make_pair(index,&(node.coeff())));
                 }
             }
@@ -6641,9 +6641,12 @@ namespace madness {
                     ar & id;
                     World* world = World::world_from_id(id.get_world_id());
                     MADNESS_ASSERT(world);
-                    ptr = static_cast< const FunctionImpl<T,NDIM>*>(world->ptr_from_id< WorldObject< FunctionImpl<T,NDIM> > >(id));
+                    auto ptr_opt = world->ptr_from_id< WorldObject< FunctionImpl<T,NDIM> > >(id);
+                    if (!ptr_opt)
+                      MADNESS_EXCEPTION("FunctionImpl: remote operation attempting to use a locally uninitialized object",0);
+                    ptr = static_cast< const FunctionImpl<T,NDIM>*>(*ptr_opt);
                     if (!ptr)
-                        MADNESS_EXCEPTION("FunctionImpl: remote operation attempting to use a locally uninitialized object",0);
+                        MADNESS_EXCEPTION("FunctionImpl: remote operation attempting to use an unregistered object",0);
                 } else {
                     ptr=nullptr;
                 }
@@ -6669,9 +6672,12 @@ namespace madness {
                     ar & id;
                     World* world = World::world_from_id(id.get_world_id());
                     MADNESS_ASSERT(world);
-                    ptr = static_cast< FunctionImpl<T,NDIM>*>(world->ptr_from_id< WorldObject< FunctionImpl<T,NDIM> > >(id));
+                    auto ptr_opt = world->ptr_from_id< WorldObject< FunctionImpl<T,NDIM> > >(id);
+                    if (!ptr_opt)
+                      MADNESS_EXCEPTION("FunctionImpl: remote operation attempting to use a locally uninitialized object",0);
+                    ptr = static_cast< FunctionImpl<T,NDIM>*>(*ptr_opt);
                     if (!ptr)
-                        MADNESS_EXCEPTION("FunctionImpl: remote operation attempting to use a locally uninitialized object",0);
+                      MADNESS_EXCEPTION("FunctionImpl: remote operation attempting to use an unregistered object",0);
                 } else {
                     ptr=nullptr;
                 }

src/madness/mra/function_common_data.h

@@ -188,7 +188,7 @@ namespace madness {
             if (found) {
                 acc->second+=time;
             } else {
-                map2.insert(std::pair<int,double>(-10,time));
+                [[maybe_unused]] auto&& [it, inserted] = map2.insert(std::pair<int,double>(-10,time));
             }
 
 
@@ -203,7 +203,7 @@ namespace madness {
             if (found) {
                 acc->second+=1.0;
             } else {
-                map2.insert(std::pair<int,long>(ilog,1));
+              [[maybe_unused]] auto&& [it, inserted] = map2.insert(std::pair<int,long>(ilog,1));
             }
         }
 

src/madness/mra/simplecache.h

@@ -90,7 +90,7 @@ namespace madness {
 
         /// Set value associated with key ... gives ownership of a new copy to the container
         inline void set(const Key<NDIM>& key, const Q& val) {
-            cache.insert(pairT(key,val));
+            [[maybe_unused]] auto&& [it, inserted] = cache.insert(pairT(key,val));
         }
 
         inline void set(Level n, Translation l, const Q& val) {

src/madness/world/future.h

@@ -531,7 +531,7 @@ namespace madness {
         /// \param[in] value The value to be assigned.
         inline void set(const T& value) {
             MADNESS_CHECK(f);
-            std::shared_ptr< FutureImpl<T> > ff = f; // manage life time of f
+            std::shared_ptr< FutureImpl<T> > ff = f; // manage lifetime of f
             ff->set(value);
         }
 
@@ -541,7 +541,7 @@ namespace madness {
         /// \param[in] value The value to be assigned.
         inline void set(T&& value) {
             MADNESS_CHECK(f);
-            std::shared_ptr< FutureImpl<T> > ff = f; // manage life time of f
+            std::shared_ptr< FutureImpl<T> > ff = f; // manage lifetime of f
             ff->set(std::move(value));
         }
 
@@ -552,7 +552,7 @@ namespace madness {
         /// \param[in] input_arch Description needed.
         inline void set(const archive::BufferInputArchive& input_arch) {
             MADNESS_CHECK(f);
-            std::shared_ptr< FutureImpl<T> > ff = f; // manage life time of f
+            std::shared_ptr< FutureImpl<T> > ff = f; // manage lifetime of f
             ff->set(input_arch);
         }
 

src/madness/world/group.cc

@@ -77,7 +77,8 @@ namespace madness {
     /// \param group The group to be removed from the registry
     void Group::unregister_group(const DistributedID& did) {
         group_registry_container::accessor acc;
-        group_registry.find(acc, did);
+        [[maybe_unused]] auto found = group_registry.find(acc, did);
+        MADNESS_ASSERT(found);
         group_registry.erase(acc);
     }
 

src/madness/world/test_hashthreaded.cc

@@ -92,7 +92,10 @@ void test_coverage() {
     if (a[-1] != -99) cout << "was expecting -99 " << a[-1] << endl;
     if (a.size() != 1) cout << "was expecting size to be 1" << endl;
 
-    for (int i=0; i<10000; ++i) a.insert(datumT(i,i*99));
+    for (int i=0; i<10000; ++i) {
+      [[maybe_unused]] auto&& [it, inserted] = a.insert(datumT(i,i*99));
+      MADNESS_ASSERT(inserted);
+    }
 
     for (int i=0; i<10000; ++i) {
         pair<iteratorT,bool> r = a.insert(datumT(i,i*99));
@@ -134,7 +137,8 @@ void test_coverage() {
     count = 10001;
     for (int i=0; i<2000; i+=2) {
         iteratorT it = a.find(i);
-        a.erase(i);
+        [[maybe_unused]] auto erased = a.try_erase(i);
+        MADNESS_ASSERT(erased);
         count--;
         if (a.size() != count) cout << "size should have been " << count << " " << a.size() << endl;
         it = a.find(i);
@@ -143,7 +147,7 @@ void test_coverage() {
 
     for (int i=2001; i<4000; i+=2) {
         iteratorT it = a.find(i);
-        if (a.erase(i) != 1) cout << "expected to have deleted one element " << i << endl;
+        if (!a.try_erase(i)) cout << "expected to have deleted one element " << i << endl;
         count--;
         if (a.size() != count) cout << "c. size should have been " << count << " " << a.size() << endl;
         it = a.find(i);
@@ -167,7 +171,10 @@ void test_coverage() {
     for (int nelem=1; nelem<=10000; nelem*=10) {
         cout << "nelem " << nelem << endl;
         a.clear();
-        for (int i=0; i<nelem; ++i) a.insert(datumT(i,i*99));
+        for (int i=0; i<nelem; ++i) {
+          [[maybe_unused]] auto&& [it, inserted] = a.insert(datumT(i,i*99));
+          MADNESS_ASSERT(inserted);
+        }
         //a.print_stats();
         if (a.size() != size_t(std::distance(a.begin(),a.end())))
             cout << "size not equal to end-start\n";
@@ -213,13 +220,15 @@ void test_time() {
             vector<int> v = random_perm(nentries);
             double insert_used = madness::cpu_time();
             for (int i=0; i<nentries; ++i) {
-                a.insert(datumT(i,i));
+                [[maybe_unused]] auto&& [it, inserted] = a.insert(datumT(i,i));
+                MADNESS_ASSERT(inserted);
             }
             insert_used = madness::cpu_time()-insert_used;
             v = random_perm(nentries);
             double del_used = madness::cpu_time();
             for (int i=0; i<nentries; ++i) {
-                a.erase(i);
+                [[maybe_unused]] auto erased = a.try_erase(i);
+                MADNESS_ASSERT(erased);
             }
             del_used = madness::cpu_time()-del_used;
             printf("nbin=%8d   nent=%8d   insert=%.1es/call   del=%.1es/call\n",
@@ -249,8 +258,8 @@ void do_test_random(ConcurrentHashMap<int,double>& a, size_t& count, double& sum
             }
         }
         else {
-            int ndel = a.erase(key);
-            if (ndel == 1) {
+            auto erased = a.try_erase(key);
+            if (erased) {
                 sum -= value;
                 count --;
             }