refactor: Add -Wshadow and fix warnings

.github/workflows/build.yml

@@ -16,7 +16,7 @@ jobs:
 
     steps:
     - uses: actions/checkout@v2
-    - run: make noomp
+    - run: make noomp -j
       env:
         CXXFLAGS: -Werror -Wno-unknown-pragmas
     - run: ./Infomap --version

CMakeLists.txt

@@ -4,7 +4,7 @@ project(infomap)
 set(CMAKE_CXX_STANDARD 14)
 set(CMAKE_CXX_EXTENSIONS OFF)
 
-set(CMAKE_CXX_FLAGS "-Wall -Wextra -pedantic")
+set(CMAKE_CXX_FLAGS "-Wall -Wextra -pedantic -Wshadow")
 set(CMAKE_CXX_FLAGS_DEBUG "-g")
 set(CMAKE_CXX_FLAGS_RELEASE "-O3")
 

Makefile

@@ -1,4 +1,4 @@
-CXXFLAGS += -Wall -Wextra -pedantic -std=c++14
+CXXFLAGS += -Wall -Wextra -pedantic -Wnon-virtual-dtor -std=c++14
 LDFLAGS +=
 CXX_CLANG := $(shell $(CXX) --version 2>/dev/null | grep clang)
 BREW := $(shell which brew 2>/dev/null)
@@ -18,7 +18,7 @@ else
 			LDFLAGS += -fopenmp
 		endif
 	else
-		CXXFLAGS += -O3
+		CXXFLAGS += -Wshadow -O3
 		ifneq "$(findstring noomp, $(MAKECMDGOALS))" "noomp"
 			CXXFLAGS += -Xpreprocessor -fopenmp
 			LDFLAGS += -lomp

src/core/InfomapBase.cpp

@@ -870,11 +870,11 @@ void InfomapBase::hierarchicalPartition()
           std::vector<InfoNode*> subModules(numLeafs, nullptr);
           module.releaseChildren();
 
-          for (auto i = 0; i < numLeafs; ++i) {
-            InfoNode* leaf = leafs[i];
-            unsigned int moduleIndex = modules[i];
+          for (auto j = 0; j < numLeafs; ++j) {
+            InfoNode* leaf = leafs[j];
+            unsigned int moduleIndex = modules[j];
             if (subModules[moduleIndex] == nullptr) {
-              subModules[moduleIndex] = new InfoNode(subInfomap.leafNodes()[i]->parent->data);
+              subModules[moduleIndex] = new InfoNode(subInfomap.leafNodes()[j]->parent->data);
               subModules[moduleIndex]->index = moduleIndex;
               module.addChild(subModules[moduleIndex]);
             }

src/core/MemMapEquation.cpp

@@ -360,19 +360,20 @@ void MemMapEquation::updatePhysicalNodes(InfoNode& current, unsigned int oldModu
     auto overlapIt = moduleToMemNodes.find(oldModuleIndex);
     if (overlapIt == moduleToMemNodes.end())
       throw std::length_error(io::Str() << "Couldn't find old module " << oldModuleIndex << " in physical node " << physData.physNodeIndex);
-    MemNodeSet& memNodeSet = overlapIt->second;
-    memNodeSet.sumFlow -= physData.sumFlowFromM2Node;
-    if (--memNodeSet.numMemNodes == 0)
+
+    MemNodeSet& oldMemNodeSet = overlapIt->second;
+    oldMemNodeSet.sumFlow -= physData.sumFlowFromM2Node;
+    if (--oldMemNodeSet.numMemNodes == 0)
       moduleToMemNodes.erase(overlapIt);
 
     // Add contribution to new module
     overlapIt = moduleToMemNodes.find(bestModuleIndex);
-    if (overlapIt == moduleToMemNodes.end())
+    if (overlapIt == moduleToMemNodes.end()) {
       moduleToMemNodes.insert(std::make_pair(bestModuleIndex, MemNodeSet(1, physData.sumFlowFromM2Node)));
-    else {
-      MemNodeSet& memNodeSet = overlapIt->second;
-      ++memNodeSet.numMemNodes;
-      memNodeSet.sumFlow += physData.sumFlowFromM2Node;
+    } else {
+      MemNodeSet& newMemNodeSet = overlapIt->second;
+      ++newMemNodeSet.numMemNodes;
+      newMemNodeSet.sumFlow += physData.sumFlowFromM2Node;
     }
   }
 }
@@ -390,13 +391,14 @@ void MemMapEquation::addMemoryContributionsAndUpdatePhysicalNodes(InfoNode& curr
     auto overlapIt = moduleToMemNodes.find(oldModuleIndex);
     if (overlapIt == moduleToMemNodes.end())
       throw std::length_error("Couldn't find old module among physical node assignments.");
-    MemNodeSet& memNodeSet = overlapIt->second;
-    double oldPhysFlow = memNodeSet.sumFlow;
-    double newPhysFlow = memNodeSet.sumFlow - physData.sumFlowFromM2Node;
+
+    MemNodeSet& oldMemNodeSet = overlapIt->second;
+    double oldPhysFlow = oldMemNodeSet.sumFlow;
+    double newPhysFlow = oldMemNodeSet.sumFlow - physData.sumFlowFromM2Node;
     oldModuleDelta.sumDeltaPlogpPhysFlow += infomath::plogp(newPhysFlow) - infomath::plogp(oldPhysFlow);
     oldModuleDelta.sumPlogpPhysFlow += infomath::plogp(physData.sumFlowFromM2Node);
-    memNodeSet.sumFlow -= physData.sumFlowFromM2Node;
-    if (--memNodeSet.numMemNodes == 0)
+    oldMemNodeSet.sumFlow -= physData.sumFlowFromM2Node;
+    if (--oldMemNodeSet.numMemNodes == 0)
       moduleToMemNodes.erase(overlapIt);
 
     // Add contribution to new module
@@ -408,13 +410,13 @@ void MemMapEquation::addMemoryContributionsAndUpdatePhysicalNodes(InfoNode& curr
       newModuleDelta.sumDeltaPlogpPhysFlow += infomath::plogp(newPhysFlow) - infomath::plogp(oldPhysFlow);
       newModuleDelta.sumPlogpPhysFlow += infomath::plogp(physData.sumFlowFromM2Node);
     } else {
-      MemNodeSet& memNodeSet = overlapIt->second;
-      oldPhysFlow = memNodeSet.sumFlow;
-      newPhysFlow = memNodeSet.sumFlow + physData.sumFlowFromM2Node;
+      MemNodeSet& newMemNodeSet = overlapIt->second;
+      oldPhysFlow = newMemNodeSet.sumFlow;
+      newPhysFlow = newMemNodeSet.sumFlow + physData.sumFlowFromM2Node;
       newModuleDelta.sumDeltaPlogpPhysFlow += infomath::plogp(newPhysFlow) - infomath::plogp(oldPhysFlow);
       newModuleDelta.sumPlogpPhysFlow += infomath::plogp(physData.sumFlowFromM2Node);
-      ++memNodeSet.numMemNodes;
-      memNodeSet.sumFlow += physData.sumFlowFromM2Node;
+      ++newMemNodeSet.numMemNodes;
+      newMemNodeSet.sumFlow += physData.sumFlowFromM2Node;
     }
   }
 }

src/core/MetaMapEquation.cpp

@@ -153,9 +153,9 @@ double MetaMapEquation::calcCodelengthOnModuleOfLeafNodes(const InfoNode& parent
       metaCollection.add(node.metaData[0], weightByFlow ? node.data.flow : m_unweightedNodeFlow); // TODO: Initiate to collection and use all dimensions
   }
 
-  double metaCodelength = metaCollection.calculateEntropy();
+  double _metaCodelength = metaCollection.calculateEntropy();
 
-  return indexLength + metaDataRate * metaCodelength;
+  return indexLength + metaDataRate * _metaCodelength;
 }
 
 double MetaMapEquation::getDeltaCodelengthOnMovingNode(InfoNode& current,

src/io/ProgramInterface.cpp

@@ -230,9 +230,12 @@ void ProgramInterface::parseArgs(const std::string& args)
   // Split the flags on whitespace
   std::vector<std::string> flags;
   std::istringstream argStream(args);
-  std::string arg;
-  while (!(argStream >> arg).fail())
-    flags.push_back(arg);
+
+  {
+    std::string arg;
+    while (!(argStream >> arg).fail())
+      flags.push_back(arg);
+  }
 
   std::deque<std::string> nonOpts;
   try {

src/utils/FileURI.cpp

@@ -19,8 +19,8 @@ namespace infomap {
 FileURI::FileURI(string filename, bool requireExtension)
     : m_filename(std::move(filename)), m_requireExtension(requireExtension)
 {
-  auto getErrorMessage = [](const auto& filename, auto requireExt) {
-    string s = io::Str() << "Filename '" << filename << "' must match the pattern \"[dir/]name" << (requireExt ? ".extension\"" : "[.extension]\"");
+  auto getErrorMessage = [](const auto& name, auto requireExt) {
+    string s = io::Str() << "Filename '" << name << "' must match the pattern \"[dir/]name" << (requireExt ? ".extension\"" : "[.extension]\"");
     return s;
   };
 

src/utils/FlowCalculator.cpp

@@ -305,7 +305,7 @@ void FlowCalculator::calcDirectedFlow(const StateNetwork& network, const Config&
   double danglingRank;
 
   // Calculate PageRank
-  const auto iteration = [&](const auto iteration, const double alpha, const double beta) {
+  const auto iteration = [&](const auto iter, const double alpha, const double beta) {
     danglingRank = std::accumulate(cbegin(nodeFlow), cbegin(nodeFlow) + nonDanglingStartIndex, 0.0);
 
     // Flow from teleportation
@@ -331,7 +331,7 @@ void FlowCalculator::calcDirectedFlow(const StateNetwork& network, const Config&
 
     // Normalize if needed
     if (std::abs(nodeFlowDiff) > 1.0e-10) {
-      Log() << "(Normalizing ranks after " << iteration << " power iterations with error " << nodeFlowDiff << ") ";
+      Log() << "(Normalizing ranks after " << iter << " power iterations with error " << nodeFlowDiff << ") ";
       normalize(nodeFlow, nodeFlowDiff + 1.0);
     }
 
@@ -437,7 +437,7 @@ void FlowCalculator::calcDirectedRegularizedFlow(const StateNetwork& network, co
   std::vector<double> nodeFlowTmp(numNodes, 0.0);
 
   // Calculate PageRank
-  const auto iteration = [&](const auto iteration) {
+  const auto iteration = [&](const auto iter) {
     double teleTmp = 0.0;
     for (unsigned int i = 0; i < N; ++i) {
       teleTmp += alpha[i] * nodeFlow[i];
@@ -465,7 +465,7 @@ void FlowCalculator::calcDirectedRegularizedFlow(const StateNetwork& network, co
 
     // Normalize if needed
     if (std::abs(nodeFlowDiff) > 1.0e-10) {
-      Log() << "(Normalizing ranks after " << iteration << " power iterations with error " << nodeFlowDiff << ") ";
+      Log() << "(Normalizing ranks after " << iter << " power iterations with error " << nodeFlowDiff << ") ";
       normalize(nodeFlow, nodeFlowDiff + 1.0);
     }
 
@@ -637,7 +637,7 @@ void FlowCalculator::calcDirectedBipartiteFlow(const StateNetwork& network, cons
   double danglingRank;
 
   // Calculate two-step PageRank
-  const auto iteration = [&](const auto iteration, const double alpha, const double beta) {
+  const auto iteration = [&](const auto iter, const double alpha, const double beta) {
     danglingRank = 0.0;
     for (const auto& i : danglingIndices) {
       danglingRank += nodeFlow[i];
@@ -676,7 +676,7 @@ void FlowCalculator::calcDirectedBipartiteFlow(const StateNetwork& network, cons
 
     // Normalize if needed
     if (std::abs(nodeFlowDiff) > 1.0e-10) {
-      Log() << "(Normalizing ranks after " << iteration << " power iterations with error " << nodeFlowDiff << ") ";
+      Log() << "(Normalizing ranks after " << iter << " power iterations with error " << nodeFlowDiff << ") ";
       normalize(nodeFlow, nodeFlowDiff + 1.0);
     }