Use std::unique_ptr for memory managment of readers

CMakeLists.txt

@@ -1,7 +1,7 @@
 cmake_minimum_required(VERSION 3.16)
 project(omnireader LANGUAGES C CXX)
 
-set(CMAKE_CXX_STANDARD 11)
+set(CMAKE_CXX_STANDARD 14)
 
 if (${BUILD_PYTHON})
     find_package(PythonExtensions REQUIRED)

libomnireader/include/omnireader.h

@@ -7,6 +7,7 @@
 
 #include <string>
 #include <cstring>
+#include <memory>
 
 namespace OmniReader {
 
@@ -67,7 +68,7 @@ namespace OmniReader {
         virtual inline unsigned long long fill_page(unsigned long long remainder) = 0;
     };
 
-    Reader *GetReader(Format option);
+    std::unique_ptr<Reader> GetReader(Format option);
 }
 
 #endif //OMNIREADER_OMNIREADER_H

libomnireader/src/omnireader.cpp

@@ -2,6 +2,7 @@
 #include <cstring>
 #include <cstdlib>
 #include <iostream>
+#include <memory>
 
 #include "omnireader.h"
 
@@ -104,14 +105,14 @@ namespace OmniReader {
 #include "GZReader.h"
 
 namespace OmniReader {
-    Reader *GetReader(OmniReader::Format option) {
+    std::unique_ptr<Reader> GetReader(OmniReader::Format option) {
       switch (option) {
         case OmniReader::Format::PlainText:
-          return new PlainTextReader();
+          return  std::make_unique<PlainTextReader>(PlainTextReader());
         case OmniReader::Format::BZ2:
-          return new BZ2Reader();
+          return std::make_unique<BZ2Reader>(BZ2Reader());
         case OmniReader::Format::GZ:
-          return new GZReader();
+          return std::make_unique<GZReader>(GZReader());
         default:
           return nullptr;
       }

libomnireader/test/test_lines.cpp

@@ -9,7 +9,7 @@
 
 using namespace OmniReader;
 
-std::vector<int> PrintLines(Reader* r) {
+std::vector<int> PrintLines( const std::unique_ptr<Reader> &r) {
   std::vector<int> lengths;
 
   while (!r->at_eof()) {
@@ -48,33 +48,30 @@ int main(int argc, char* argv[]) {
 
   const char* fname = argv[1];
 
-  OmniReader::Reader* r1 = GetReader(Format::PlainText);
+  auto r1 = GetReader(Format::PlainText);
   if (!r1->open(fname)) {
     fprintf(stderr, "Failed to open file: %s\n", argv[1]);
     return 1;
   }
   std::vector<int> ref_lengths = PrintLines(r1);
-  delete r1;
 
   std::string bz2fname(fname);
   bz2fname.append(".bz2");
-  OmniReader::Reader* r2 = GetReader(Format::BZ2);
+  auto r2 = GetReader(Format::BZ2);
   if (!r2->open(bz2fname.c_str())) {
     fprintf(stderr, "Failed to open bz2 file\n");
     return 1;
   }
   std::vector<int> bz2_lengths = PrintLines(r2);
-  delete r2;
 
   std::string gzfname(fname);
   gzfname.append(".gz");
-  OmniReader::Reader* r3 = GetReader(Format::GZ);
+  auto r3 = GetReader(Format::GZ);
   if (!r3->open(gzfname.c_str())) {
     fprintf(stderr, "Failed to open gz file\n");
     return 1;
   }
   std::vector<int> gz_lengths = PrintLines(r3);
-  delete r3;
 
   if (!compare_lengths(ref_lengths, gz_lengths)) {
     fputs("Failed for GZip\n", stderr);

libomnireader/test/test_seekntell.cpp

@@ -16,7 +16,7 @@ int main(int argc, const char* argv[]) {
 
   const char* fname = argv[1];
 
-  OmniReader::Reader* r = OmniReader::GetReader(OmniReader::Format::PlainText);
+  auto r = OmniReader::GetReader(OmniReader::Format::PlainText);
   r->open(fname);
 
   std::cout << r->tell() << "\t";

omnireader/CMakeLists.txt

@@ -1,3 +1,6 @@
+# find fast_float
+find_package(fast_float REQUIRED)
+
 add_cython_target(groreader CXX PY3 groreader.pyx)
 add_library(groreader MODULE ${groreader})
 python_extension_module(groreader)

omnireader/groreader.pyx

@@ -5,6 +5,8 @@ from libcpp cimport bool
 from libcpp.string cimport string as stdstring
 from cython.operator import dereference
 cimport cython
+from libcpp.memory cimport unique_ptr
+
 
 import numpy as np
 from MDAnalysis.core.topologyattrs import (
@@ -53,10 +55,8 @@ cdef object stripwhitespace(const char* ptr, const char* end):
 
 
 cdef class GROReader:
-    cdef Reader* r
+    cdef unique_ptr[Reader] r
 
-    def __cinit__(self):
-        self.r = NULL
 
     def __init__(self, str fname):
         if fname.endswith('bz2'):
@@ -66,12 +66,9 @@ cdef class GROReader:
         else:
             self.r = GetReader(Format.PlainText)
 
-        if not self.r.open(fname):
+        if not self.r.get().open(fname):
             raise ValueError
 
-    def __dealloc__(self):
-        if (self.r != NULL):
-            del self.r
 
     @cython.wraparound(False)
     @cython.boundscheck(False)
@@ -81,17 +78,17 @@ cdef class GROReader:
         cdef float tmp
         cdef const char* ptr
 
-        self.r.advance()
-        lptr = self.r.line_start()
+        self.r.get().advance()
+        lptr = self.r.get().line_start()
         natoms = atoi(lptr)
-        self.r.advance()
+        self.r.get().advance()
 
         out = np.empty(natoms * 3, dtype=np.float32, order='C')
         cdef float [::1] coords = out
 
         i = 0
-        while not self.r.at_eof():
-            lptr = self.r.line_start()
+        while not self.r.get().at_eof():
+            lptr = self.r.get().line_start()
 
             if (i == 0):  # find spacing between coordinates
                 cs = (strchr(lptr + 25, 46) - (lptr + 25)) + 1  # 46 == .
@@ -105,7 +102,7 @@ cdef class GROReader:
             i += 1
             if i == natoms:
                 break
-            self.r.advance()
+            self.r.get().advance()
 
         return out.reshape(-1, 3)
 
@@ -120,9 +117,9 @@ cdef class GROReader:
         cdef object[::1] names_view
         cdef unsigned int[::1] indices_view
 
-        self.r.advance()
-        ptr = self.r.line_start()
-        self.r.advance()
+        self.r.get().advance()
+        ptr = self.r.get().line_start()
+        self.r.get().advance()
         natoms = atoi(ptr)
 
         resids = np.empty(natoms, dtype=np.uint32)
@@ -138,8 +135,8 @@ cdef class GROReader:
         cdef const char* lptr
         # [:5], [5:10], [10:15], [15:20]
         i = 0
-        while not self.r.at_eof():
-            lptr = self.r.line_start()
+        while not self.r.get().at_eof():
+            lptr = self.r.get().line_start()
 
             resids_view[i] = strtoint(lptr, lptr + 5)
             resnames_view[i] = stripwhitespace(lptr + 5, lptr + 10)
@@ -149,7 +146,7 @@ cdef class GROReader:
             i += 1
             if (i == natoms):
                 break
-            self.r.advance()
+            self.r.get().advance()
 
         return resids, resnames, names, indices
 

omnireader/libomnireader.pxd

@@ -1,5 +1,6 @@
 from libcpp cimport bool
 from libcpp.string cimport string as stdstring
+from libcpp.memory cimport unique_ptr
 
 
 cdef extern from "Python.h":
@@ -24,7 +25,7 @@ cdef extern from "omnireader.h" namespace "OmniReader":
         unsigned long long tell()
         bool rewind()
 
-    Reader* GetReader(int f)
+    unique_ptr[Reader] GetReader(int f)
 
 
 cdef extern from "fast_float.h" namespace "fast_float":
@@ -34,7 +35,7 @@ cdef extern from "fast_float.h" namespace "fast_float":
     from_chars_result from_chars[T, UC](const UC *start, const UC* end, T& result)
 
 
-cdef unsigned int strtoint(const char* beg, const char* end):
+cdef inline unsigned int strtoint(const char* beg, const char* end):
     # fixed format file, so ints could be touching other ints, so strtoi can't be used
     # e.g. indices column could be touching the positions column
     cdef unsigned int ret = 0

omnireader/linesupplier.pyx

@@ -1,6 +1,7 @@
 
 from libcpp cimport bool
 from libcpp.string cimport string as stdstring
+from libcpp.memory cimport unique_ptr
 
 import cython
 import numpy as np
@@ -11,10 +12,8 @@ from omnireader.libomnireader cimport (
 
 
 cdef class LineSupplier:
-    cdef Reader *r
+    cdef unique_ptr[Reader] r
 
-    def __cinit__(self):
-        self.r = NULL
 
     def __init__(self, str fname):
         if fname.endswith('bz2'):
@@ -24,20 +23,17 @@ cdef class LineSupplier:
         else:
             self.r = GetReader(Format.PlainText)
 
-        if not self.r.open(bytes(fname, 'utf-8')):
+        if not self.r.get().open(bytes(fname, 'utf-8')):
             raise ValueError
 
-    def __dealloc__(self):
-        if self.r != NULL:
-            del self.r
 
     def get_line(self):
-        cdef stdstring s = self.r.get_line()
+        cdef stdstring s = self.r.get().get_line()
 
         return s
 
     def advance(self):
         cdef bool ret
-        ret = self.r.advance()
+        ret = self.r.get().advance()
 
         return ret