Mark constructors as explicit

Summary: Without the `explicit` keyword, we allow implicit conversions, which are error-prone. Let's fix that. Reviewed By: arnaudvenet Differential Revision: D49460439 fbshipit-source-id: 143523b2876e1670323e0627b3587bd4abfa38e5
facebook · Sep 21, 2023 · 1eb4825 · 1eb4825
1 parent 3de6eed
commit 1eb4825
Show file tree

Hide file tree

Showing 12 changed files with 46 additions and 38 deletions.
diff --git a/include/sparta/HashedAbstractEnvironment.h b/include/sparta/HashedAbstractEnvironment.h
@@ -82,10 +82,10 @@ class HashedAbstractEnvironment final
   HashedAbstractEnvironment()
       : AbstractDomainScaffolding<Value, HashedAbstractEnvironment>() {}
 
-  HashedAbstractEnvironment(AbstractValueKind kind)
+  explicit HashedAbstractEnvironment(AbstractValueKind kind)
       : AbstractDomainScaffolding<Value, HashedAbstractEnvironment>(kind) {}
 
-  HashedAbstractEnvironment(
+  explicit HashedAbstractEnvironment(
       std::initializer_list<std::pair<Variable, Domain>> l) {
     for (const auto& p : l) {
       if (p.second.is_bottom()) {

diff --git a/include/sparta/HashedAbstractPartition.h b/include/sparta/HashedAbstractPartition.h
@@ -51,7 +51,8 @@ class HashedAbstractPartition final
    */
   HashedAbstractPartition() = default;
 
-  HashedAbstractPartition(std::initializer_list<std::pair<Label, Domain>> l) {
+  explicit HashedAbstractPartition(
+      std::initializer_list<std::pair<Label, Domain>> l) {
     for (const auto& p : l) {
       set(p.first, p.second);
     }

diff --git a/include/sparta/HashedSetAbstractDomain.h b/include/sparta/HashedSetAbstractDomain.h
@@ -35,9 +35,9 @@ class SetValue final : public PowersetImplementation<
 
   SetValue() = default;
 
-  SetValue(Element e) { add(std::move(e)); }
+  explicit SetValue(Element e) { add(std::move(e)); }
 
-  SetValue(std::initializer_list<Element> l) {
+  explicit SetValue(std::initializer_list<Element> l) {
     if (l.begin() != l.end()) {
       m_set = std::make_unique<SetImplType>(l.begin(), l.end());
     }
@@ -193,7 +193,7 @@ class HashedSetAbstractDomain final
                                const std::unordered_set<Element, Hash, Equal>&,
                                HashedSetAbstractDomain>() {}
 
-  HashedSetAbstractDomain(AbstractValueKind kind)
+  explicit HashedSetAbstractDomain(AbstractValueKind kind)
       : PowersetAbstractDomain<Element,
                                Value,
                                const std::unordered_set<Element, Hash, Equal>&,

diff --git a/include/sparta/MonotonicFixpointIterator.h b/include/sparta/MonotonicFixpointIterator.h
@@ -116,7 +116,8 @@ class MonotonicFixpointIteratorBase
    * the constructor, so as to prevent unnecessary resizing of the underlying
    * hashtables during the iteration.
    */
-  MonotonicFixpointIteratorBase(const Graph& graph, size_t cfg_size_hint = 4)
+  explicit MonotonicFixpointIteratorBase(const Graph& graph,
+                                         size_t cfg_size_hint = 4)
       : m_graph(graph),
         m_entry_states(cfg_size_hint),
         m_exit_states(cfg_size_hint) {}
@@ -220,7 +221,7 @@ class SuccessorNodeListBuilder {
   using NodeId = typename GraphInterface::NodeId;
 
  public:
-  SuccessorNodeListBuilder(const Graph& graph) : m_graph(graph) {}
+  explicit SuccessorNodeListBuilder(const Graph& graph) : m_graph(graph) {}
 
   std::vector<NodeId> operator()(const NodeId& x) {
     const auto& succ_edges = GraphInterface::successors(m_graph, x);
@@ -277,7 +278,8 @@ class WTOMonotonicFixpointIterator
   using Context =
       fp_impl::MonotonicFixpointIteratorContext<NodeId, Domain, NodeHash>;
 
-  WTOMonotonicFixpointIterator(const Graph& graph, size_t cfg_size_hint = 4)
+  explicit WTOMonotonicFixpointIterator(const Graph& graph,
+                                        size_t cfg_size_hint = 4)
       : fp_impl::MonotonicFixpointIteratorBase<GraphInterface,
                                                Domain,
                                                NodeHash>(graph, cfg_size_hint),
@@ -376,7 +378,7 @@ class ParallelMonotonicFixpointIterator
       fp_impl::MonotonicFixpointIteratorContext<NodeId, Domain, NodeHash>;
   using WPOWorkerState = WorkerState<uint32_t>;
 
-  ParallelMonotonicFixpointIterator(
+  explicit ParallelMonotonicFixpointIterator(
       const Graph& graph, size_t num_thread = parallel::default_num_threads())
       : fp_impl::
             MonotonicFixpointIteratorBase<GraphInterface, Domain, NodeHash>(
@@ -572,7 +574,8 @@ class MonotonicFixpointIterator
   using Context =
       fp_impl::MonotonicFixpointIteratorContext<NodeId, Domain, NodeHash>;
 
-  MonotonicFixpointIterator(const Graph& graph, size_t cfg_size_hint = 4)
+  explicit MonotonicFixpointIterator(const Graph& graph,
+                                     size_t cfg_size_hint = 4)
       : fp_impl::MonotonicFixpointIteratorBase<GraphInterface,
                                                Domain,
                                                NodeHash>(graph, cfg_size_hint),

diff --git a/include/sparta/PatriciaTreeMapAbstractEnvironment.h b/include/sparta/PatriciaTreeMapAbstractEnvironment.h
@@ -56,11 +56,11 @@ class PatriciaTreeMapAbstractEnvironment final
       : AbstractDomainScaffolding<Value, PatriciaTreeMapAbstractEnvironment>() {
   }
 
-  PatriciaTreeMapAbstractEnvironment(AbstractValueKind kind)
+  explicit PatriciaTreeMapAbstractEnvironment(AbstractValueKind kind)
       : AbstractDomainScaffolding<Value, PatriciaTreeMapAbstractEnvironment>(
             kind) {}
 
-  PatriciaTreeMapAbstractEnvironment(
+  explicit PatriciaTreeMapAbstractEnvironment(
       std::initializer_list<std::pair<Variable, Domain>> l) {
     for (const auto& p : l) {
       if (p.second.is_bottom()) {

diff --git a/include/sparta/PatriciaTreeMapAbstractPartition.h b/include/sparta/PatriciaTreeMapAbstractPartition.h
@@ -60,7 +60,7 @@ class PatriciaTreeMapAbstractPartition final
    */
   PatriciaTreeMapAbstractPartition() = default;
 
-  PatriciaTreeMapAbstractPartition(
+  explicit PatriciaTreeMapAbstractPartition(
       std::initializer_list<std::pair<Label, Domain>> l) {
     for (const auto& p : l) {
       set(p.first, p.second);

diff --git a/include/sparta/PatriciaTreeOverUnderSetAbstractDomain.h b/include/sparta/PatriciaTreeOverUnderSetAbstractDomain.h
@@ -23,13 +23,13 @@ class OverUnderSetValue final
  public:
   OverUnderSetValue() = default;
 
-  OverUnderSetValue(Element e)
+  explicit OverUnderSetValue(Element e)
       : OverUnderSetValue(PatriciaTreeSet<Element>{std::move(e)}) {}
 
-  OverUnderSetValue(std::initializer_list<Element> l)
+  explicit OverUnderSetValue(std::initializer_list<Element> l)
       : OverUnderSetValue(PatriciaTreeSet<Element>(l)) {}
 
-  OverUnderSetValue(PatriciaTreeSet<Element> over_and_under)
+  explicit OverUnderSetValue(PatriciaTreeSet<Element> over_and_under)
       : m_over(over_and_under), m_under(std::move(over_and_under)) {
     // Union is unnecessary.
   }

diff --git a/include/sparta/PatriciaTreeSetAbstractDomain.h b/include/sparta/PatriciaTreeSetAbstractDomain.h
@@ -32,11 +32,12 @@ class SetValue final
  public:
   SetValue() = default;
 
-  SetValue(Element e) : m_set(std::move(e)) {}
+  explicit SetValue(Element e) : m_set(std::move(e)) {}
 
-  SetValue(std::initializer_list<Element> l) : m_set(l.begin(), l.end()) {}
+  explicit SetValue(std::initializer_list<Element> l)
+      : m_set(l.begin(), l.end()) {}
 
-  SetValue(PatriciaTreeSet<Element> set) : m_set(std::move(set)) {}
+  explicit SetValue(PatriciaTreeSet<Element> set) : m_set(std::move(set)) {}
 
   const PatriciaTreeSet<Element>& elements() const { return m_set; }
 
@@ -129,7 +130,7 @@ class PatriciaTreeSetAbstractDomain final
                                const PatriciaTreeSet<Element>&,
                                PatriciaTreeSetAbstractDomain>() {}
 
-  PatriciaTreeSetAbstractDomain(AbstractValueKind kind)
+  explicit PatriciaTreeSetAbstractDomain(AbstractValueKind kind)
       : PowersetAbstractDomain<Element,
                                Value,
                                const PatriciaTreeSet<Element>&,

diff --git a/include/sparta/SmallSortedSetAbstractDomain.h b/include/sparta/SmallSortedSetAbstractDomain.h
@@ -22,7 +22,7 @@ class SetValue final : public AbstractValue<SetValue<Element, MaxCount>> {
  public:
   SetValue() = default;
 
-  SetValue(FlatSet<Element> set) : m_set(std::move(set)) {}
+  explicit SetValue(FlatSet<Element> set) : m_set(std::move(set)) {}
 
   void clear() { m_set.clear(); }
 

diff --git a/include/sparta/SparseSetAbstractDomain.h b/include/sparta/SparseSetAbstractDomain.h
@@ -42,7 +42,7 @@ class SparseSetValue final
   SparseSetValue() : m_capacity(0), m_element_num(0) {}
 
   // Returns an empty set over a universe of the given size.
-  SparseSetValue(size_t max_size)
+  explicit SparseSetValue(size_t max_size)
       : m_capacity(max_size),
         m_element_num(0),
         m_dense(max_size),

diff --git a/include/sparta/WorkQueue.h b/include/sparta/WorkQueue.h
@@ -192,16 +192,17 @@ class WorkQueue {
   const bool m_can_push_task{false};
 
  public:
-  WorkQueue(Executor,
-            unsigned int num_threads = parallel::default_num_threads(),
-            // push_tasks_while_running:
-            // * When this flag is true, all threads stay alive until the
-            //   last task is finished. Useful when threads are adding
-            //   more work to the queue via `WorkerState::push_task`.
-            // * When this flag is false, threads can
-            //   exit as soon as there is no more work (to avoid
-            //   preempting a thread that has useful work)
-            bool push_tasks_while_running = false);
+  explicit WorkQueue(Executor,
+                     unsigned int num_threads = parallel::default_num_threads(),
+                     // push_tasks_while_running:
+                     // * When this flag is true, all threads stay alive until
+                     //   the last task is finished. Useful when threads are
+                     //   adding more work to the queue via
+                     //   `WorkerState::push_task`.
+                     // * When this flag is false, threads can
+                     //   exit as soon as there is no more work (to avoid
+                     //   preempting a thread that has useful work)
+                     bool push_tasks_while_running = false);
 
   // copies are not allowed
   WorkQueue(const WorkQueue&) = delete;

diff --git a/test/DisjointUnionAbstractDomainTest.cpp b/test/DisjointUnionAbstractDomainTest.cpp
@@ -30,24 +30,26 @@ INSTANTIATE_TYPED_TEST_CASE_P(DisjointUnionAbstractDomain,
 template <>
 std::vector<IntStringDomain>
 AbstractDomainPropertyTest<IntStringDomain>::top_values() {
-  return {IntDomain::top(), StringDomain::top()};
+  return {IntStringDomain{IntDomain::top()},
+          IntStringDomain{StringDomain::top()}};
 }
 
 template <>
 std::vector<IntStringDomain>
 AbstractDomainPropertyTest<IntStringDomain>::bottom_values() {
-  return {IntDomain::bottom(), StringDomain::bottom()};
+  return {IntStringDomain{IntDomain::bottom()},
+          IntStringDomain{StringDomain::bottom()}};
 }
 
 template <>
 std::vector<IntStringDomain>
 AbstractDomainPropertyTest<IntStringDomain>::non_extremal_values() {
-  return {IntDomain(0), StringDomain("foo")};
+  return {IntStringDomain{IntDomain(0)}, IntStringDomain{StringDomain("foo")}};
 }
 
 TEST(DisjointUnionAbstractDomainTest, basicOperations) {
-  IntStringDomain zero = IntDomain(0);
-  IntStringDomain str = StringDomain("");
+  auto zero = IntStringDomain{IntDomain(0)};
+  auto str = IntStringDomain{StringDomain("")};
   EXPECT_TRUE(zero.join(str).is_top());
   EXPECT_TRUE(zero.meet(str).is_bottom());
   EXPECT_NLEQ(zero, str);