From 3beea372d4084fa3ffc2a94e26916c9ee5f0f031 Mon Sep 17 00:00:00 2001
From: Nathan Young <92858834+TactfulDeity@users.noreply.github.com>
Date: Thu, 14 Nov 2024 19:30:41 -0500
Subject: [PATCH] address code review feedback

Signed-off-by: Nathan Young <92858834+TactfulDeity@users.noreply.github.com>
---
 avogadro/core/color3f.h            | 11 +++++++++--
 avogadro/core/crystaltools.cpp     |  2 +-
 avogadro/core/gaussiansettools.cpp |  3 ++-
 avogadro/core/layer.cpp            |  2 +-
 avogadro/core/molecule.cpp         |  4 ++--
 avogadro/core/ringperceiver.cpp    | 25 ++++++++++++++++++++++++-
 avogadro/core/slaterset.h          |  2 +-
 avogadro/core/slatersettools.cpp   |  5 +++--
 8 files changed, 43 insertions(+), 11 deletions(-)

diff --git a/avogadro/core/color3f.h b/avogadro/core/color3f.h
index ffbc1b582a..6f30c0b00d 100644
--- a/avogadro/core/color3f.h
+++ b/avogadro/core/color3f.h
@@ -9,6 +9,10 @@
 #include <array>
 
 namespace Avogadro::Core {
+namespace detail {
+/** Used to represent Maximum RGB value as a float. */
+constexpr float k_MaxRGBColorValue = 255.0f;
+} // namespace detail
 
 /**
  * @class Color3f color3f.h <avogadro/core/color3f.h>
@@ -87,11 +91,14 @@ class Color3f
 
 inline Color3f::Color3f(float r, float g, float b) : m_data({ r, g, b }) {}
 
+// clang-format off
 inline Color3f::Color3f(int r, int g, int b)
-  : m_data({ static_cast<float>(r) / 255.0f, static_cast<float>(g) / 255.0f,
-             static_cast<float>(b) / 255.0f })
+: m_data({static_cast<float>(r) / detail::k_MaxRGBColorValue,
+          static_cast<float>(g) / detail::k_MaxRGBColorValue,
+          static_cast<float>(b) / detail::k_MaxRGBColorValue})
 {
 }
+// clang-format on
 
 inline void Color3f::set(float r, float g, float b)
 {
diff --git a/avogadro/core/crystaltools.cpp b/avogadro/core/crystaltools.cpp
index 700fa2a943..a662e42858 100644
--- a/avogadro/core/crystaltools.cpp
+++ b/avogadro/core/crystaltools.cpp
@@ -256,7 +256,7 @@ bool CrystalTools::niggliReduce(Molecule& molecule, Options opts)
 */
 #define NIGGLI_DEBUG(step)
 
-  // Allow ADL for swap
+  // Allow Argument Dependent Lookup for swap
   using std::swap;
 
   // Perform iterative reduction:
diff --git a/avogadro/core/gaussiansettools.cpp b/avogadro/core/gaussiansettools.cpp
index 97d20021dc..fd5a80ef44 100644
--- a/avogadro/core/gaussiansettools.cpp
+++ b/avogadro/core/gaussiansettools.cpp
@@ -121,7 +121,8 @@ double GaussianSetTools::calculateSpinDensity(const Vector3& position) const
 
 bool GaussianSetTools::isValid() const
 {
-  return m_molecule && dynamic_cast<GaussianSet*>(m_molecule->basisSet());
+  return (m_molecule != nullptr) &&
+         (dynamic_cast<GaussianSet*>(m_molecule->basisSet()) != nullptr);
 }
 
 inline bool GaussianSetTools::isSmall(double val) const
diff --git a/avogadro/core/layer.cpp b/avogadro/core/layer.cpp
index c3993e157c..e6b3837a47 100644
--- a/avogadro/core/layer.cpp
+++ b/avogadro/core/layer.cpp
@@ -108,7 +108,7 @@ void Layer::removeLayer(size_t layer)
 
 void Layer::swapLayer(Index a, Index b)
 {
-  // Allow ADL for swap
+  // Allow Argument Dependent Lookup for swap
   using std::swap;
 
   swap(m_atomAndLayers[a], m_atomAndLayers[b]);
diff --git a/avogadro/core/molecule.cpp b/avogadro/core/molecule.cpp
index b1865ae4c7..eed67bf9c5 100644
--- a/avogadro/core/molecule.cpp
+++ b/avogadro/core/molecule.cpp
@@ -564,7 +564,7 @@ Molecule::AtomType Molecule::addAtom(unsigned char number, Vector3 position3d)
 
 void Molecule::swapBond(Index a, Index b)
 {
-  // Allow ADL for swap
+  // Allow Argument Dependent Lookup for swap
   using std::swap;
 
   m_graph.swapEdgeIndices(a, b);
@@ -572,7 +572,7 @@ void Molecule::swapBond(Index a, Index b)
 }
 void Molecule::swapAtom(Index a, Index b)
 {
-  // Allow ADL for swap
+  // Allow Argument Dependent Lookup for swap
   using std::swap;
 
   Index max = a > b ? a : b;
diff --git a/avogadro/core/ringperceiver.cpp b/avogadro/core/ringperceiver.cpp
index cac91ac742..f6c49cdbd2 100644
--- a/avogadro/core/ringperceiver.cpp
+++ b/avogadro/core/ringperceiver.cpp
@@ -22,11 +22,26 @@ namespace {
 class DistanceMatrix
 {
 public:
+  // swap function marked friend so it can only be found via Argument Dependent
+  // Lookup (ADL)
+  friend void swap(DistanceMatrix& first, DistanceMatrix& second)
+  {
+    // Enable ADL for the swap
+    using std::swap;
+
+    swap(first.m_size, second.m_size);
+    swap(first.m_values, second.m_values);
+  }
+
   // construction and destruction
   DistanceMatrix(size_t size);
-  DistanceMatrix(const DistanceMatrix& other);
   ~DistanceMatrix();
 
+  DistanceMatrix(const DistanceMatrix& other);
+
+  // intentional pass-by-value to leverage previous copy ctor
+  DistanceMatrix& operator=(DistanceMatrix other);
+
   // operators
   size_t operator()(size_t i, size_t j) const;
   size_t& operator()(size_t i, size_t j);
@@ -51,6 +66,14 @@ DistanceMatrix::DistanceMatrix(const DistanceMatrix& other)
               m_values);
 }
 
+DistanceMatrix& DistanceMatrix::operator=(DistanceMatrix other)
+{
+  // will use friend swap function via Argument Dependent Lookup
+  swap(*this, other);
+
+  return *this;
+}
+
 DistanceMatrix::~DistanceMatrix()
 {
   delete[] m_values;
diff --git a/avogadro/core/slaterset.h b/avogadro/core/slaterset.h
index 582edea64a..619c074eb7 100644
--- a/avogadro/core/slaterset.h
+++ b/avogadro/core/slaterset.h
@@ -140,7 +140,7 @@ class AVOGADROCORE_EXPORT SlaterSet : public BasisSet
   MatrixX& normalizedMatrix() { return m_normalized; }
   MatrixX& densityMatrix() { return m_density; }
 
-  // Does nothing?
+  // Nonfunctional - Included for backwards compatibility
   void outputAll();
 
 private:
diff --git a/avogadro/core/slatersettools.cpp b/avogadro/core/slatersettools.cpp
index ea618b70e5..61ad6c94eb 100644
--- a/avogadro/core/slatersettools.cpp
+++ b/avogadro/core/slatersettools.cpp
@@ -65,12 +65,13 @@ double SlaterSetTools::calculateSpinDensity(const Vector3&) const
 
 bool SlaterSetTools::isValid() const
 {
-  return m_molecule && dynamic_cast<SlaterSet*>(m_molecule->basisSet());
+  return (m_molecule != nullptr) &&
+         (dynamic_cast<SlaterSet*>(m_molecule->basisSet()) != nullptr);
 }
 
 inline bool SlaterSetTools::isSmall(double val) const
 {
-  return val > -1e-20 && val < 1e-20;
+  return std::abs(val) < 1e-20;
 }
 
 std::vector<double> SlaterSetTools::calculateValues(