Move many loops over glcr::Vector to range-based loops.

lib/glacier/container/vector.h

@@ -37,6 +37,7 @@ class Vector {
 
   // Setters.
   // FIXME: Handle downsizing.
+  // TODO: Rename this so it is clear that this only affects capacity.
   void Resize(uint64_t capacity);
 
   void PushBack(const T& item);
@@ -47,6 +48,44 @@ class Vector {
 
   T&& PopBack();
 
+  // Forward Iter
+  class Iterator {
+   public:
+    Iterator(T* item, uint64_t size) : item_(item), size_(size) {}
+
+    Iterator next() {
+      if (size_ <= 1) {
+        return {nullptr, 0};
+      }
+      return {item_ + 1, size_ - 1};
+    }
+
+    Iterator& operator++() {
+      if (size_ <= 1) {
+        item_ = nullptr;
+        size_ = 0;
+      } else {
+        item_++;
+        size_--;
+      }
+      return *this;
+    }
+
+    T& operator*() { return *item_; }
+    T* operator->() { return item_; }
+    bool operator==(const Iterator& other) { return item_ == other.item_; }
+    bool operator!=(const Iterator& other) { return item_ != other.item_; }
+
+   private:
+    T* item_;
+    uint64_t size_;
+  };
+
+  Iterator begin() { return {data_, size_}; }
+  const Iterator begin() const { return {data_, size_}; }
+  Iterator end() { return {nullptr, 0}; }
+  const Iterator end() const { return {nullptr, 0}; }
+
  private:
   T* data_;
   uint64_t size_;

lib/glacier/test/CMakeLists.txt

@@ -1,4 +1,6 @@
 find_package(Catch2 3 REQUIRED)
+find_program(MEMORYCHECK_COMMAND valgrind)
+set(MEMORYCHECK_COMMAND_OPTIONS "--trace-children=yes --leak-check=full")
 
 add_subdirectory(container)
 

lib/glacier/test/container/vector.cpp

@@ -4,7 +4,169 @@
 
 using namespace glcr;
 
-TEST_CASE("Build Vector", "[vector]") {
+TEST_CASE("Empty Vector", "[vector]") {
   Vector<uint64_t> v;
   REQUIRE(v.size() == 0);
+  REQUIRE(v.empty());
+}
+
+TEST_CASE("Push/Pop Vector", "[vector]") {
+  Vector<uint64_t> v;
+  v.PushBack(42);
+  REQUIRE(v.size() == 1);
+  REQUIRE(v.capacity() >= 1);
+  v.PushBack(33);
+  REQUIRE(v.size() == 2);
+  REQUIRE(v.capacity() >= 2);
+
+  REQUIRE(v.at(0) == 42);
+  REQUIRE(v[0] == 42);
+  REQUIRE(v.at(1) == 33);
+  REQUIRE(v[1] == 33);
+
+  REQUIRE(v.PopBack() == 33);
+  REQUIRE(v.size() == 1);
+  REQUIRE(v.PopBack() == 42);
+  REQUIRE(v.size() == 0);
+}
+
+class ConstructRecorder {
+ public:
+  static uint64_t construct_cnt;
+  static uint64_t copy_cnt;
+  static uint64_t move_cnt;
+  ConstructRecorder() { construct_cnt++; }
+  ConstructRecorder(const ConstructRecorder&) { copy_cnt++; }
+  ConstructRecorder& operator=(const ConstructRecorder&) {
+    copy_cnt++;
+    return *this;
+  }
+
+  ConstructRecorder(ConstructRecorder&&) { move_cnt++; }
+  ConstructRecorder& operator=(ConstructRecorder&&) {
+    move_cnt++;
+    return *this;
+  }
+
+  static void Reset() {
+    construct_cnt = 0;
+    copy_cnt = 0;
+    move_cnt = 0;
+  }
+
+ private:
+  uint64_t dummy_data = 0;
+};
+
+uint64_t ConstructRecorder::construct_cnt = 0;
+uint64_t ConstructRecorder::copy_cnt = 0;
+uint64_t ConstructRecorder::move_cnt = 0;
+
+TEST_CASE("Data-Type Construction", "[vector]") {
+  ConstructRecorder::Reset();
+  Vector<ConstructRecorder> v;
+
+  SECTION("Copy Insert") {
+    ConstructRecorder obj;
+    v.PushBack(obj);
+    // This is overfitted on the implementation which also default constructs
+    // the held objects when allocating a new backing array.
+    REQUIRE(ConstructRecorder::construct_cnt == 2);
+    REQUIRE(ConstructRecorder::copy_cnt == 1);
+    REQUIRE(ConstructRecorder::move_cnt == 0);
+  }
+
+  SECTION("Move Insert") {
+    ConstructRecorder obj;
+    v.PushBack(glcr::Move(obj));
+    // This is overfitted on the implementation which also default constructs
+    // the held objects when allocating a new backing array.
+    REQUIRE(ConstructRecorder::construct_cnt == 2);
+    REQUIRE(ConstructRecorder::copy_cnt == 0);
+    REQUIRE(ConstructRecorder::move_cnt == 1);
+  }
+
+  SECTION("RValue Insert") {
+    v.PushBack({});
+    // This is overfitted on the implementation which also default constructs
+    // the held objects when allocating a new backing array.
+    REQUIRE(ConstructRecorder::construct_cnt == 2);
+    REQUIRE(ConstructRecorder::copy_cnt == 0);
+    REQUIRE(ConstructRecorder::move_cnt == 1);
+  }
+
+  SECTION("Emplace Insert") {
+    v.EmplaceBack();
+    // This is overfitted on the implementation which also default constructs
+    // the held objects when allocating a new backing array.
+    REQUIRE(ConstructRecorder::construct_cnt == 2);
+    REQUIRE(ConstructRecorder::copy_cnt == 0);
+    REQUIRE(ConstructRecorder::move_cnt == 1);
+  }
+
+  SECTION("PopBack Move") {
+    v.EmplaceBack();
+    ConstructRecorder obj = v.PopBack();
+
+    // This is overfitted on the implementation which also default constructs
+    // the held objects when allocating a new backing array.
+    REQUIRE(ConstructRecorder::construct_cnt == 2);
+    REQUIRE(ConstructRecorder::copy_cnt == 0);
+    // 1 from emplace, 1 from pop. (No additional regular constructions).
+    REQUIRE(ConstructRecorder::move_cnt == 2);
+  }
+}
+
+TEST_CASE("Vector Move", "[vector]") {
+  ConstructRecorder::Reset();
+
+  Vector<ConstructRecorder> v;
+  v.PushBack({});
+  v.PushBack({});
+  v.PushBack({});
+
+  uint64_t construct = ConstructRecorder::construct_cnt;
+  uint64_t copy = ConstructRecorder::copy_cnt;
+  uint64_t move = ConstructRecorder::move_cnt;
+
+  Vector<ConstructRecorder> v2(glcr::Move(v));
+
+  REQUIRE(v2.size() == 3);
+  REQUIRE(v2.capacity() >= 3);
+  REQUIRE(ConstructRecorder::construct_cnt == construct);
+  REQUIRE(ConstructRecorder::copy_cnt == copy);
+  REQUIRE(ConstructRecorder::move_cnt == move);
+
+  Vector<ConstructRecorder> v3 = glcr::Move(v2);
+
+  REQUIRE(v3.size() == 3);
+  REQUIRE(v3.capacity() >= 3);
+  REQUIRE(ConstructRecorder::construct_cnt == construct);
+  REQUIRE(ConstructRecorder::copy_cnt == copy);
+  REQUIRE(ConstructRecorder::move_cnt == move);
+}
+
+TEST_CASE("Vector Iterator", "[vector]") {
+  Vector<uint64_t> v;
+  for (uint64_t i = 0; i < 100; i++) {
+    v.PushBack(42);
+  }
+
+  SECTION("For Range Loop") {
+    uint64_t iters = 0;
+    for (uint64_t i : v) {
+      REQUIRE(i == 42);
+      iters++;
+    }
+    REQUIRE(iters == 100);
+  }
+
+  SECTION("Raw Iter Loop") {
+    uint64_t iters = 0;
+    for (auto it = v.begin(); it != v.end(); ++it) {
+      REQUIRE(*it == 42);
+      iters++;
+    }
+    REQUIRE(iters == 100);
+  }
 }

lib/mammoth/file/file.cpp

@@ -72,8 +72,8 @@ glcr::ErrorOr<glcr::Vector<glcr::String>> ListDirectory(glcr::StringView path) {
 
   auto file_views = glcr::StrSplit(dir.filenames(), ',');
   glcr::Vector<glcr::String> files;
-  for (uint64_t i = 0; i < file_views.size(); i++) {
-    files.PushBack(file_views[i]);
+  for (const auto& view : glcr::StrSplit(dir.filenames(), ',')) {
+    files.PushBack(view);
   }
   return files;
 }

scripts/test-mem.sh

@@ -0,0 +1,16 @@
+#! /bin/bash
+
+set -e
+
+DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+
+pushd "$DIR/.."
+if  [[ ! -e test-bin  ]]; then
+  cmake -B test-bin/ -G Ninja -D enable_testing=on
+fi
+pushd test-bin/
+ninja build_test
+ctest --output-on-failure -T memcheck
+popd
+popd
+

scripts/test.sh

@@ -5,7 +5,9 @@ set -e
 DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
 
 pushd "$DIR/.."
-cmake -B test-bin/ -G Ninja -D enable_testing=on
+if  [[ ! -e test-bin  ]]; then
+  cmake -B test-bin/ -G Ninja -D enable_testing=on
+fi
 pushd test-bin/
 ninja build_test
 ctest --output-on-failure

sys/denali/denali_server.cpp

@@ -39,8 +39,8 @@ glcr::Status DenaliServer::HandleReadMany(const ReadManyRequest& req,
   }
 
   uint64_t sector_cnt = 0;
-  for (uint64_t i = 0; i < req.sector_cnt().size(); i++) {
-    sector_cnt += req.sector_cnt().at(i);
+  for (uint64_t cnt : req.sector_cnt()) {
+    sector_cnt += cnt;
   }
   uint64_t region_paddr;
   mmth::OwnedMemoryRegion region = mmth::OwnedMemoryRegion::ContiguousPhysical(

sys/teton/terminal.cpp

@@ -55,9 +55,8 @@ void Terminal::ExecuteCommand(const glcr::String& command) {
     if (!files_or.ok()) {
       console_.WriteString(glcr::StrFormat("Error: {}\n", files_or.error()));
     } else {
-      auto& files = files_or.value();
-      for (uint64_t i = 0; i < files.size(); i++) {
-        console_.WriteString(files[i]);
+      for (const auto& file : files_or.value()) {
+        console_.WriteString(file);
         console_.WriteChar('\n');
       }
     }

sys/victoriafalls/victoriafalls_server.cpp

@@ -94,8 +94,8 @@ glcr::Status VFSServer::HandleGetDirectory(const GetDirectoryRequest& request,
   }
 
   glcr::VariableStringBuilder filelist;
-  for (uint64_t i = 0; i < files.size(); i++) {
-    filelist.PushBack(glcr::StringView(files.at(i).name, files.at(i).name_len));
+  for (const DirEntry& file : files) {
+    filelist.PushBack(glcr::StringView(file.name, file.name_len));
     filelist.PushBack(',');
   }
   // Remove trailing comma.

sys/yellowstone/yellowstone.cpp

@@ -47,10 +47,9 @@ uint64_t main(uint64_t port_cap) {
   glcr::Vector<glcr::StringView> files =
       glcr::StrSplit(init_file.as_str(), '\n');
 
-  for (uint64_t i = 0; i < files.size(); i++) {
-    if (!files[i].empty()) {
-      mmth::File binary =
-          mmth::File::Open(glcr::StrFormat("/bin/{}", files[i]));
+  for (glcr::StringView& file : files) {
+    if (!file.empty()) {
+      mmth::File binary = mmth::File::Open(glcr::StrFormat("/bin/{}", file));
 
       ASSIGN_OR_RETURN(client_cap, server->CreateClientCap());
       auto error_or = mmth::SpawnProcessFromElfRegion(

zion/object/process.cpp

@@ -65,9 +65,9 @@ void Process::Exit(uint64_t exit_code) {
   state_ = CLEANUP;
   exit_code_ = exit_code;
 
-  for (uint64_t i = 0; i < threads_.size(); i++) {
-    if (!threads_[i]->IsDying()) {
-      threads_[i]->SetState(Thread::CLEANUP);
+  for (const auto& t : threads_) {
+    if (!t->IsDying()) {
+      t->SetState(Thread::CLEANUP);
     }
   }
 
@@ -93,9 +93,9 @@ void Process::Cleanup() {
   }
 
   // 1. For each thread, call cleanup.
-  for (uint64_t i = 0; i < threads_.size(); i++) {
-    if (threads_[i]->GetState() == Thread::CLEANUP) {
-      threads_[i]->Cleanup();
+  for (const auto& t : threads_) {
+    if (t->GetState() == Thread::CLEANUP) {
+      t->Cleanup();
     }
   }