]> https://gitweb.dealii.org/ - dealii.git/commitdiff
Disallow using rvalue references for Subscriptor::(un)?subscribe 7440/head
authorDaniel Arndt <daniel.arndt@iwr.uni-heidelberg.de>
Mon, 12 Nov 2018 17:25:43 +0000 (18:25 +0100)
committerDaniel Arndt <daniel.arndt@iwr.uni-heidelberg.de>
Tue, 8 Jan 2019 09:43:32 +0000 (10:43 +0100)
doc/news/changes/incompatibilities/20181112DanielArndt [new file with mode: 0644]
include/deal.II/base/subscriptor.h
include/deal.II/distributed/tria.h
source/base/subscriptor.cc
tests/base/unsubscribe_subscriptor.cc
tests/base/unsubscribe_subscriptor_01.cc [new file with mode: 0644]
tests/base/unsubscribe_subscriptor_01.output [new file with mode: 0644]

diff --git a/doc/news/changes/incompatibilities/20181112DanielArndt b/doc/news/changes/incompatibilities/20181112DanielArndt
new file mode 100644 (file)
index 0000000..398177d
--- /dev/null
@@ -0,0 +1,8 @@
+Changed: Subscriptor::subscribe cannot be called with rvalue references anymore.
+In particular, temporary string literals cannot be used as argument. Storing
+string literals with the same content at the same memory location is
+implementation defined and cannot be relied upon. On the other hand, the
+subscriber strings are not compared by their memory address anymore but by their
+actual content.
+<br>
+(Daniel Arndt, 2018/11/12)
index e1d4baca6ecb116f3de968411db0cc764eee03b6..c41b315ba3b2917874697680e81cde85d1f0ef6f 100644 (file)
@@ -22,6 +22,7 @@
 #include <deal.II/base/exceptions.h>
 
 #include <atomic>
+#include <cstring>
 #include <map>
 #include <mutex>
 #include <string>
@@ -104,11 +105,23 @@ public:
 
   /**
    * Subscribes a user of the object by storing the pointer @p validity. The
-   * subscriber may be identified by text supplied as @p identifier.
+   * subscriber may be identified by text supplied as @p identifier. The latter
+   * variable must not be a temporary and its type must decay to
+   * const char *. In particular, calling this function with a rvalue reference
+   * is not allowed.
+   */
+  template <typename ConstCharStar = const char *>
+  typename std::enable_if<
+    std::is_same<ConstCharStar, const char *>::value>::type
+  subscribe(std::atomic<bool> *const validity,
+            ConstCharStar            identifier = nullptr) const;
+
+  /**
+   * Calling subscribe() with a rvalue reference as identifier is not allowed.
    */
   void
   subscribe(std::atomic<bool> *const validity,
-            const char *             identifier = nullptr) const;
+            const char *&&           identifier) const = delete;
 
   /**
    * Unsubscribes a user from the object.
@@ -191,16 +204,6 @@ public:
   serialize(Archive &ar, const unsigned int version);
 
 private:
-  /**
-   * The data type used in #counter_map.
-   */
-  using map_value_type = std::map<const char *, unsigned int>::value_type;
-
-  /**
-   * The iterator type used in #counter_map.
-   */
-  using map_iterator = std::map<const char *, unsigned int>::iterator;
-
   /**
    * Store the number of objects which subscribed to this object. Initially,
    * this number is zero, and upon destruction it shall be zero again (i.e.
@@ -219,11 +222,34 @@ private:
    */
   mutable std::atomic<unsigned int> counter;
 
+  /*
+   * Functor struct used for key comparison in #counter_map.
+   * Not the memory location but the actual C-string content is compared.
+   */
+  struct MapCompare
+  {
+    bool
+    operator()(const char *lhs, const char *rhs) const
+    {
+      return std::strcmp(lhs, rhs) > 0;
+    }
+  };
+
   /**
    * In this map, we count subscriptions for each different identification
    * string supplied to subscribe().
    */
-  mutable std::map<const char *, unsigned int> counter_map;
+  mutable std::map<const char *, unsigned int, MapCompare> counter_map;
+
+  /**
+   * The data type used in #counter_map.
+   */
+  using map_value_type = decltype(counter_map)::value_type;
+
+  /**
+   * The iterator type used in #counter_map.
+   */
+  using map_iterator = decltype(counter_map)::iterator;
 
   /**
    * In this vector, we store pointers to the validity bool in the SmartPointer
index 161fb6443132dfb2938f6333aa2c4793049ee3e3..04b212427be60e9fb92032117bc42119ea393ab9 100644 (file)
@@ -442,7 +442,7 @@ namespace parallel
        * Override the implementation of prepare_coarsening_and_refinement from
        * the base class. This is necessary if periodic boundaries are enabled
        * and the level difference over vertices over the periodic boundary
-       * must be not more than 2:1.
+       * must not be more than 2:1.
        */
       virtual bool
       prepare_coarsening_and_refinement() override;
index abde0083a0946eae85523acf6ed5163d19dcb1b8..eec6f7305b8182f5dd3b6ff7550ba50d31369072 100644 (file)
@@ -153,8 +153,10 @@ Subscriptor::operator=(Subscriptor &&s) noexcept
 
 
 
+template <>
 void
-Subscriptor::subscribe(std::atomic<bool> *const validity, const char *id) const
+Subscriptor::subscribe<const char *>(std::atomic<bool> *const validity,
+                                     const char *             id) const
 {
   std::lock_guard<std::mutex> lock(mutex);
 
@@ -175,30 +177,34 @@ Subscriptor::subscribe(std::atomic<bool> *const validity, const char *id) const
 }
 
 
+
 void
 Subscriptor::unsubscribe(std::atomic<bool> *const validity,
                          const char *             id) const
 {
   const char *name = (id != nullptr) ? id : unknown_subscriber;
-  AssertNothrow(counter > 0, ExcNoSubscriber(object_info->name(), name));
-  // This is for the case that we do
-  // not abort after the exception
   if (counter == 0)
-    return;
+    {
+      AssertNothrow(counter > 0, ExcNoSubscriber(object_info->name(), name));
+      // This is for the case that we do not abort after the exception
+      return;
+    }
 
   std::lock_guard<std::mutex> lock(mutex);
 
   map_iterator it = counter_map.find(name);
-  if (it == counter_map.end() || it->second == 0)
+  if (it == counter_map.end())
     {
       AssertNothrow(it != counter_map.end(),
                     ExcNoSubscriber(object_info->name(), name));
-      AssertNothrow(it->second > 0, ExcNoSubscriber(object_info->name(), name));
+      // This is for the case that we do not abort after the exception
+      return;
     }
-  else
+  if (it->second == 0)
     {
-      --counter;
-      it->second--;
+      AssertNothrow(it->second > 0, ExcNoSubscriber(object_info->name(), name));
+      // This is for the case that we do not abort after the exception
+      return;
     }
 
   auto validity_ptr_it =
@@ -209,9 +215,12 @@ Subscriptor::unsubscribe(std::atomic<bool> *const validity,
         validity_ptr_it != validity_pointers.end(),
         ExcMessage(
           "This Subscriptor object does not know anything about the supplied pointer!"));
+      return;
     }
-  else
-    validity_pointers.erase(validity_ptr_it);
+
+  validity_pointers.erase(validity_ptr_it);
+  --counter;
+  --it->second;
 }
 
 
index 271f3badd93a12a547efe796940a8ed7811d098f..a1c13f9c03250d0c512cb65cf99dd0016e208461 100644 (file)
@@ -38,9 +38,13 @@ main()
 
   Subscriptor       subscriptor;
   std::atomic<bool> dummy_a;
-  subscriptor.subscribe(&dummy_a, "a");
-  subscriptor.unsubscribe(&dummy_a, "b");
-  subscriptor.unsubscribe(&dummy_a, "a");
+  const char *      foo_a = "a";
+  const char *      foo_b = "b";
+  subscriptor.subscribe(&dummy_a, foo_a);
+  subscriptor.unsubscribe(&dummy_a, foo_b);
+  std::atomic<bool> dummy_b;
+  subscriptor.unsubscribe(&dummy_b, foo_a);
+  subscriptor.unsubscribe(&dummy_a, foo_a);
 
   return 0;
 }
diff --git a/tests/base/unsubscribe_subscriptor_01.cc b/tests/base/unsubscribe_subscriptor_01.cc
new file mode 100644 (file)
index 0000000..d418385
--- /dev/null
@@ -0,0 +1,56 @@
+// ---------------------------------------------------------------------
+//
+// Copyright (C) 2018 by the deal.II authors
+//
+// This file is part of the deal.II library.
+//
+// The deal.II library is free software; you can use it, redistribute
+// it, and/or modify it under the terms of the GNU Lesser General
+// Public License as published by the Free Software Foundation; either
+// version 2.1 of the License, or (at your option) any later version.
+// The full text of the license can be found in the file LICENSE.md at
+// the top level directory of deal.II.
+//
+// ---------------------------------------------------------------------
+
+
+
+// check that unsubscribing with a wrong id is handled correctly. This time,
+// we check that unsubscribung with a different pointer with the same content
+// works as well
+
+
+#include <deal.II/base/smartpointer.h>
+#include <deal.II/base/subscriptor.h>
+
+#include <iostream>
+#include <vector>
+
+#include "../tests.h"
+
+class Test : public Subscriptor
+{};
+
+int
+main()
+{
+  deal_II_exceptions::disable_abort_on_exception();
+
+  initlog();
+
+  Subscriptor       subscriptor;
+  std::atomic<bool> dummy_a;
+  const char *      foo        = "a";
+  const std::string foo_string = "a";
+  subscriptor.subscribe(&dummy_a, foo);
+  subscriptor.unsubscribe(&dummy_a, foo_string.c_str());
+
+  deallog << "OK" << std::endl;
+
+  subscriptor.subscribe(&dummy_a, foo);
+  subscriptor.unsubscribe(&dummy_a, "a");
+
+  deallog << "OK" << std::endl;
+
+  return 0;
+}
diff --git a/tests/base/unsubscribe_subscriptor_01.output b/tests/base/unsubscribe_subscriptor_01.output
new file mode 100644 (file)
index 0000000..8b3b075
--- /dev/null
@@ -0,0 +1,3 @@
+
+DEAL::OK
+DEAL::OK

In the beginning the Universe was created. This has made a lot of people very angry and has been widely regarded as a bad move.

Douglas Adams


Typeset in Trocchi and Trocchi Bold Sans Serif.