Merge pull request #14 from slsdetectorgroup/warnings

Warnings

CMakeLists.txt

@@ -7,34 +7,61 @@ project(aare
     LANGUAGES C CXX 
 )
 
-add_library(aare_compiler_flags INTERFACE)
-target_compile_features(aare_compiler_flags INTERFACE cxx_std_17)
-
 cmake_policy(SET CMP0135 NEW)
 cmake_policy(SET CMP0079 NEW)
 
 include(GNUInstallDirs)
 include(FetchContent)
 
+#Set default build type if none was specified
+include(cmake/helpers.cmake)
+default_build_type("Release")
 
 option(AARE_USE_WARNINGS "Eable warnings" ON)
 option(AARE_PYTHON_BINDINGS "Build python bindings" ON)
 option(AARE_TESTS "Build tests" ON)
 option(AARE_EXAMPLES "Build examples" ON)
-option(AARE_DEBUG "Compile in debug mode" ON) 
+
 
 
 set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
 find_package(fmt 6 REQUIRED)
 
-if (AARE_DEBUG)
+add_library(aare_compiler_flags INTERFACE)
-    target_compile_options(aare_compiler_flags INTERFACE -Og -ggdb3 -D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC)
+target_compile_features(aare_compiler_flags INTERFACE cxx_std_17)
-else()
+
+#TODO! Explicitly setting flags is not cross platform compatible
+if(CMAKE_BUILD_TYPE STREQUAL "Release")
+    message(STATUS "Release build")
     target_compile_options(aare_compiler_flags INTERFACE -O3)
+else()
+    target_compile_options(
+        aare_compiler_flags 
+        INTERFACE 
+            -Og 
+            -ggdb3 
+            -D_GLIBCXX_DEBUG 
+            -D_GLIBCXX_DEBUG_PEDANTIC
+    )
 endif()
 
 if(AARE_USE_WARNINGS)
-    target_compile_options(aare_compiler_flags INTERFACE -Wall -Wextra -pedantic -Wshadow )
+    target_compile_options(
+        aare_compiler_flags 
+        INTERFACE 
+            -Wall 
+            -Wextra 
+            -pedantic 
+            -Wshadow 
+            -Wnon-virtual-dtor
+            -Woverloaded-virtual
+            -Wdouble-promotion
+            -Wformat=2
+            -Wredundant-decls
+            -Wvla
+            -Wdouble-promotion
+            -Werror=return-type #important can cause segfault in optimzed builds
+        )
 endif()
 
 if(AARE_BUILD_TESTS)

core/include/aare/Frame.hpp

@@ -1,35 +1,37 @@
 #pragma once
-#include <cstddef>
-#include <sys/types.h>
-#include <cstdint>
-#include <bits/unique_ptr.h>
-#include <vector>
 #include "aare/defs.hpp"
-
+#include <bits/unique_ptr.h>
-
+#include <cstddef>
+#include <cstdint>
+#include <sys/types.h>
+#include <vector>
 
 /**
  * @brief Frame class to represent a single frame of data
  * model class
  * should be able to work with streams coming from files or network
-*/
+ */
 
+template <class DataType> class Frame {
+    ssize_t m_rows;
+    ssize_t m_cols;
+    DataType *m_data;
+    ssize_t m_bitdepth = sizeof(DataType) * 8;
 
-template <class DataType> class Frame{
+  public:
-
+    Frame(std::byte *fp, ssize_t rows, ssize_t cols);
-    public:
-    ssize_t rows;
-    ssize_t cols;
-    DataType* data;
-    ssize_t bitdepth = sizeof(DataType)*8;
-
-    Frame(std::byte* fp, ssize_t rows, ssize_t cols);
     DataType get(int row, int col);
-
+    ssize_t rows() const{
-    ~Frame(){
+        return m_rows;
-        delete[] data;
+    }
+    ssize_t cols() const{
+        return m_cols;
+    }
+    ssize_t bitdepth() const{
+        return m_bitdepth;
     }
 
+    ~Frame() { delete[] m_data; }
 };
 
 typedef Frame<uint16_t> Frame16;

core/src/Frame.cpp

@@ -3,20 +3,19 @@
 
 template <typename DataType>
 Frame<DataType>::Frame(std::byte* bytes, ssize_t rows, ssize_t cols):
-  rows(rows), cols(cols) {
+  m_rows(rows), m_cols(cols) {
-    data = new DataType[rows*cols];
+    m_data = new DataType[rows*cols];
-    std::memcpy(data, bytes, rows*cols*sizeof(DataType));
+    std::memcpy(m_data, bytes, m_rows*m_cols*sizeof(DataType));
   }
 
 
-
 template <typename DataType>
 DataType Frame<DataType>::get(int row, int col) {
-  if (row < 0 || row >= rows || col < 0 || col >= cols) {
+  if (row < 0 || row >= m_rows || col < 0 || col >= m_cols) {
     std::cerr << "Invalid row or column index" << std::endl;
     return 0;
   }
-  return data[row*cols + col];
+  return m_data[row*m_cols + col];
 }
 
 

file_io/include/aare/File.hpp

@@ -17,6 +17,7 @@ class File {
   private:
 
   public:
+    virtual ~File() = default;
     std::filesystem::path fname;
     std::filesystem::path base_path;
     std::string base_name, ext;

file_io/include/aare/FileFactory.hpp

@@ -6,13 +6,14 @@ class FileFactory{
     // Class that will be used to create File objects
     // follows the factory pattern
     protected:
-    std::filesystem::path fpath;
+    std::filesystem::path m_fpath;
 public:
     static FileFactory<detector,DataType>* get_factory(std::filesystem::path);
     // virtual int deleteFile() = 0;
     virtual File<detector,DataType>* load_file()=0;//TODO: add option to load all file to memory or keep it on disk
     virtual void parse_metadata(File<detector,DataType>*)=0;
     virtual void parse_fname(File<detector,DataType>*)=0;
+    virtual ~FileFactory() = default;
 
 
 

file_io/include/aare/SubFile.hpp

@@ -7,7 +7,12 @@
 class SubFile {
   protected:
     FILE *fp = nullptr;
-    uint16_t bitdepth;
+    uint16_t m_bitdepth;
+    std::filesystem::path m_fname;
+    ssize_t m_rows{};
+    ssize_t m_cols{};
+    ssize_t n_frames{};
+    int m_sub_file_index_{};
 
   public:
     // pointer to a read_impl function. pointer will be set to the appropriate read_impl function in the constructor
@@ -22,11 +27,7 @@ class SubFile {
     size_t get_frame(std::byte *buffer, int frame_number);
 
     // TODO: define the inlines as variables and assign them in constructor
-    inline size_t bytes_per_frame() { return (bitdepth / 8) * rows * cols; }
+    inline size_t bytes_per_frame() { return (m_bitdepth / 8) * m_rows * m_cols; }
-    inline size_t pixels_per_frame() { return rows * cols; }
+    inline size_t pixels_per_frame() { return m_rows * m_cols; }
-    std::filesystem::path fname;
+
-    ssize_t rows{};
-    ssize_t cols{};
-    ssize_t n_frames{};
-    int sub_file_index_{};
 };

file_io/src/JsonFileFactory.cpp

@@ -13,7 +13,7 @@ template <DetectorType detector,typename DataType>
 JsonFileFactory<detector,DataType>::JsonFileFactory(std::filesystem::path fpath) {
     if (not is_master_file(fpath))
         throw std::runtime_error("Json file is not a master file");
-    this->fpath = fpath;
+    this->m_fpath = fpath;
 }
 
 template <DetectorType detector,typename DataType>
@@ -63,7 +63,7 @@ void JsonFileFactory<detector,DataType>::open_subfiles(File<detector,DataType> *
 template <DetectorType detector,typename DataType>
 File<detector,DataType> *JsonFileFactory<detector,DataType>::load_file() {
     JsonFile<detector,DataType> *file = new JsonFile<detector,DataType>();
-    file->fname = this->fpath;
+    file->fname = this->m_fpath;
     this->parse_fname(file);
     this->parse_metadata(file);
     file->find_number_of_subfiles();
@@ -113,9 +113,9 @@ void JsonFileFactory<detector, DataType>::find_geometry(File<detector, DataType>
 template <DetectorType detector, typename DataType>
 void JsonFileFactory<detector, DataType>::parse_fname(File<detector, DataType> *file) {
 
-    file->base_path = this->fpath.parent_path();
+    file->base_path = this->m_fpath.parent_path();
-    file->base_name = this->fpath.stem();
+    file->base_name = this->m_fpath.stem();
-    file->ext = this->fpath.extension();
+    file->ext = this->m_fpath.extension();
 
     auto pos = file->base_name.rfind("_");
     file->findex = std::stoi(file->base_name.substr(pos + 1));

file_io/src/SubFile.cpp

@@ -9,10 +9,10 @@
  */
 
 SubFile::SubFile(std::filesystem::path fname, DetectorType detector, ssize_t rows, ssize_t cols, uint16_t bitdepth) {
-    this->rows = rows;
+    this->m_rows = rows;
-    this->cols = cols;
+    this->m_cols = cols;
-    this->fname = fname;
+    this->m_fname = fname;
-    this->bitdepth = bitdepth;
+    this->m_bitdepth = bitdepth;
     fp = fopen(fname.c_str(), "rb");
     if (fp == nullptr) {
         throw std::runtime_error("Could not open file " + fname.string());
@@ -77,12 +77,12 @@ template <typename DataType> size_t SubFile::read_impl_flip(std::byte *buffer) {
     size_t rc = fread(reinterpret_cast<char *>(&tmp[0]), this->bytes_per_frame(), 1, this->fp);
 
     // copy to place
-    const size_t start = this->cols * (this->rows - 1) * sizeof(DataType);
+    const size_t start = this->m_cols * (this->m_rows - 1) * sizeof(DataType);
-    const size_t row_size = this->cols * sizeof(DataType);
+    const size_t row_size = this->m_cols * sizeof(DataType);
     auto dst = buffer + start;
     auto src = &tmp[0];
 
-    for (int i = 0; i != this->rows; ++i) {
+    for (int i = 0; i != this->m_rows; ++i) {
         memcpy(dst, src, row_size);
         dst -= row_size;
         src += row_size;

python/src/bindings.cpp

@@ -27,9 +27,9 @@ PYBIND11_MODULE(_aare, m) {
     py::class_<Frame<uint16_t>>(m, "_Frame16")
         .def(py::init<std::byte*, ssize_t, ssize_t>())
         .def("get", &Frame<uint16_t>::get)
-        .def_readonly("rows", &Frame<uint16_t>::rows)
+        .def_property_readonly("rows", &Frame<uint16_t>::rows)
-        .def_readonly("cols", &Frame<uint16_t>::cols)
+        .def_property_readonly("cols", &Frame<uint16_t>::cols)
-        .def_readonly("bitdepth", &Frame<uint16_t>::bitdepth);
+        .def_property_readonly("bitdepth", &Frame<uint16_t>::bitdepth);