From 6be4c8c9788ad0404ba47590a6aa9425f34dd567 Mon Sep 17 00:00:00 2001 From: Daniel Arndt Date: Mon, 12 Nov 2018 18:25:43 +0100 Subject: [PATCH] Disallow using rvalue references for Subscriptor::(un)?subscribe --- .../incompatibilities/20181112DanielArndt | 8 +++ include/deal.II/base/subscriptor.h | 52 ++++++++++++----- include/deal.II/distributed/tria.h | 2 +- source/base/subscriptor.cc | 33 +++++++---- tests/base/unsubscribe_subscriptor.cc | 10 +++- tests/base/unsubscribe_subscriptor_01.cc | 56 +++++++++++++++++++ tests/base/unsubscribe_subscriptor_01.output | 3 + 7 files changed, 135 insertions(+), 29 deletions(-) create mode 100644 doc/news/changes/incompatibilities/20181112DanielArndt create mode 100644 tests/base/unsubscribe_subscriptor_01.cc create mode 100644 tests/base/unsubscribe_subscriptor_01.output diff --git a/doc/news/changes/incompatibilities/20181112DanielArndt b/doc/news/changes/incompatibilities/20181112DanielArndt new file mode 100644 index 0000000000..398177d5eb --- /dev/null +++ b/doc/news/changes/incompatibilities/20181112DanielArndt @@ -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. +
+(Daniel Arndt, 2018/11/12) diff --git a/include/deal.II/base/subscriptor.h b/include/deal.II/base/subscriptor.h index e1d4baca6e..c41b315ba3 100644 --- a/include/deal.II/base/subscriptor.h +++ b/include/deal.II/base/subscriptor.h @@ -22,6 +22,7 @@ #include #include +#include #include #include #include @@ -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 std::enable_if< + std::is_same::value>::type + subscribe(std::atomic *const validity, + ConstCharStar identifier = nullptr) const; + + /** + * Calling subscribe() with a rvalue reference as identifier is not allowed. */ void subscribe(std::atomic *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::value_type; - - /** - * The iterator type used in #counter_map. - */ - using map_iterator = std::map::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 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 counter_map; + mutable std::map 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 diff --git a/include/deal.II/distributed/tria.h b/include/deal.II/distributed/tria.h index 161fb64431..04b212427b 100644 --- a/include/deal.II/distributed/tria.h +++ b/include/deal.II/distributed/tria.h @@ -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; diff --git a/source/base/subscriptor.cc b/source/base/subscriptor.cc index abde0083a0..eec6f7305b 100644 --- a/source/base/subscriptor.cc +++ b/source/base/subscriptor.cc @@ -153,8 +153,10 @@ Subscriptor::operator=(Subscriptor &&s) noexcept +template <> void -Subscriptor::subscribe(std::atomic *const validity, const char *id) const +Subscriptor::subscribe(std::atomic *const validity, + const char * id) const { std::lock_guard lock(mutex); @@ -175,30 +177,34 @@ Subscriptor::subscribe(std::atomic *const validity, const char *id) const } + void Subscriptor::unsubscribe(std::atomic *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 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 *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; } diff --git a/tests/base/unsubscribe_subscriptor.cc b/tests/base/unsubscribe_subscriptor.cc index 271f3badd9..a1c13f9c03 100644 --- a/tests/base/unsubscribe_subscriptor.cc +++ b/tests/base/unsubscribe_subscriptor.cc @@ -38,9 +38,13 @@ main() Subscriptor subscriptor; std::atomic 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 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 index 0000000000..d4183853ed --- /dev/null +++ b/tests/base/unsubscribe_subscriptor_01.cc @@ -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 +#include + +#include +#include + +#include "../tests.h" + +class Test : public Subscriptor +{}; + +int +main() +{ + deal_II_exceptions::disable_abort_on_exception(); + + initlog(); + + Subscriptor subscriptor; + std::atomic 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 index 0000000000..8b3b075900 --- /dev/null +++ b/tests/base/unsubscribe_subscriptor_01.output @@ -0,0 +1,3 @@ + +DEAL::OK +DEAL::OK -- 2.39.5