address code review feedback

Signed-off-by: Nathan Young <[email protected]>

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)
 {

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:

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

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]);

avogadro/core/molecule.cpp

@@ -564,15 +564,15 @@ 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);
   swap(m_bondOrders[a], m_bondOrders[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;

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;

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:

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(