From 60b11f6c66031cb10eecebb80c03a0f4dac5ba53 Mon Sep 17 00:00:00 2001
From: Ronen Friedman <rfriedma@redhat.com>
Date: Tue, 15 Dec 2020 12:17:44 +0200
Subject: [PATCH] S3select: cleaning clang-tidy warnings

const-correctness, overrides, extra copying, ...

Signed-off-by: Ronen Friedman <rfriedma@redhat.com>
---
 example/generate_rand_csv.c  |   3 +-
 include/s3select.h           | 152 +++++++++----------
 include/s3select_functions.h | 281 +++++++++++++++++------------------
 include/s3select_oper.h      | 151 ++++++++++---------
 test/s3select_test.cpp       |   8 +-
 5 files changed, 300 insertions(+), 295 deletions(-)

diff --git a/example/generate_rand_csv.c b/example/generate_rand_csv.c
index 67d52ada..6f279469 100644
--- a/example/generate_rand_csv.c
+++ b/example/generate_rand_csv.c
@@ -6,12 +6,11 @@ int main(int argc, char** argv)
 {
   if (argc<3)
   {
-    printf("%s <num-of-rows> <num-of-columns> \n", argv[0]);
+    printf("%s <num-of-rows> <num-of-columns>\n", argv[0]);
     return -1;
   }
 
   srand(1234);
-  int line_no=0;
   for(int i=0; i<atoi(argv[1]); i++)
   {
     printf("%d,", i);
diff --git a/include/s3select.h b/include/s3select.h
index c4e43178..02963ff4 100644
--- a/include/s3select.h
+++ b/include/s3select.h
@@ -7,6 +7,7 @@
 #include <iostream>
 #include <string>
 #include <list>
+#include <vector>
 #include "s3select_oper.h"
 #include "s3select_functions.h"
 #include "s3select_csv_parser.h"
@@ -14,6 +15,7 @@
 #include <boost/bind.hpp>
 #include <functional>
 
+using namespace std::string_literals;
 
 #define _DEBUG_TERM {string  token(a,b);std::cout << __FUNCTION__ << token << std::endl;}
 
@@ -30,7 +32,7 @@ class s3select_projections
   std::vector<base_statement*> m_projections;
 
 public:
-  bool is_aggregate()
+  bool is_aggregate() const
   {
     //TODO iterate on projections , and search for aggregate
     //for(auto p : m_projections){}
@@ -38,7 +40,7 @@ class s3select_projections
     return false;
   }
 
-  bool semantic()
+  bool semantic() const
   {
     //TODO check aggragtion function are not nested
     return false;
@@ -56,8 +58,9 @@ static s3select_reserved_word g_s3select_reserve_word;//read-only
 struct actionQ
 {
 // upon parser is accepting a token (lets say some number),
-// it push it into dedicated queue, later those tokens are poped out to build some "higher" contruct (lets say 1 + 2)
-// those containers are used only for parsing phase and not for runtime.
+// it pushes it into a dedicated queue. These tokens are later popped out to
+// build some "higher" construct (1 + 2 for example).
+// The containers below are only used during the parsing phase, not for runtime.
 
   std::vector<mulldiv_operation::muldiv_t> muldivQ;
   std::vector<addsub_operation::addsub_op_t> addsubQ;
@@ -311,13 +314,13 @@ struct s3select : public bsc::grammar<s3select>
 
   int semantic()
   {
-    for (auto e : get_projections_list())
+    for (const auto& e : get_projections_list())
     {
-      base_statement* aggr = 0;
+      const base_statement* aggr;
 
-      if ((aggr = e->get_aggregate()) != 0)
+      if ((aggr = e->get_aggregate()) != nullptr)
       {
-        if (aggr->is_nested_aggregate(aggr))
+        if (aggr->is_nested_aggregate())
         {
           error_description = "nested aggregation function is illegal i.e. sum(...sum ...)";
           throw base_s3select_exception(error_description, base_s3select_exception::s3select_exp_en_t::FATAL);
@@ -327,10 +330,10 @@ struct s3select : public bsc::grammar<s3select>
       }
     }
 
-    if (aggr_flow == true)
-      for (auto e : get_projections_list())
+    if (aggr_flow)
+      for (const auto& e : get_projections_list())
       {
-        base_statement* skip_expr = e->get_aggregate();
+        auto skip_expr = e->get_aggregate();
 
         if (e->is_binop_aggregate_and_column(skip_expr))
         {
@@ -344,7 +347,7 @@ struct s3select : public bsc::grammar<s3select>
 
   int parse_query(const char* input_query)
   {
-    if(get_projections_list().empty() == false)
+    if(!get_projections_list().empty())
     {
       return 0;  //already parsed
     }
@@ -397,7 +400,7 @@ struct s3select : public bsc::grammar<s3select>
     return  cond->semantic();
   }
 
-  std::string get_from_clause()
+  std::string get_from_clause() const
   {
     return m_actionQ.from_clause;
   }
@@ -405,7 +408,7 @@ struct s3select : public bsc::grammar<s3select>
   void load_schema(std::vector< std::string>& scm)
   {
     int i = 0;
-    for (auto c : scm)
+    for (auto& c : scm)
     {
       m_sca.set_column_pos(c.c_str(), i++);
     }
@@ -413,9 +416,9 @@ struct s3select : public bsc::grammar<s3select>
 
   base_statement* get_filter()
   {
-    if(m_actionQ.condQ.size()==0)
+    if(m_actionQ.condQ.empty())
     {
-      return NULL;
+      return nullptr;
     }
 
     return m_actionQ.condQ.back();
@@ -436,7 +439,7 @@ struct s3select : public bsc::grammar<s3select>
     return &m_actionQ.alias_map;
   }
 
-  bool is_aggregate_query()
+  bool is_aggregate_query() const
   {
     return aggr_flow == true;
   }
@@ -450,7 +453,7 @@ struct s3select : public bsc::grammar<s3select>
   template <typename ScannerT>
   struct definition
   {
-    definition(s3select const& self)
+    explicit definition(s3select const& self)
     {
       ///// s3select syntax rules and actions for building AST
 
@@ -600,7 +603,7 @@ void push_variable::operator()(s3select* self, const char* a, const char* b) con
 {
   std::string token(a, b);
 
-  variable* v = 0;
+  variable* v = nullptr;
 
   if (g_s3select_reserve_word.is_reserved_word(token))
   {
@@ -630,7 +633,7 @@ void push_addsub::operator()(s3select* self, const char* a, const char* b) const
 {
   std::string token(a, b);
 
-  if (token.compare("+") == 0)
+  if (token == "+")
   {
     self->getAction()->addsubQ.push_back(addsub_operation::addsub_op_t::ADD);
   }
@@ -644,15 +647,15 @@ void push_mulop::operator()(s3select* self, const char* a, const char* b) const
 {
   std::string token(a, b);
 
-  if (token.compare("*") == 0)
+  if (token == "*")
   {
     self->getAction()->muldivQ.push_back(mulldiv_operation::muldiv_t::MULL);
   }
-  else if (token.compare("/") == 0)
+  else if (token == "/")
   {
     self->getAction()->muldivQ.push_back(mulldiv_operation::muldiv_t::DIV);
   }
-  else if(token.compare("^") == 0)
+  else if(token == "^")
   {
     self->getAction()->muldivQ.push_back(mulldiv_operation::muldiv_t::POW);
   }
@@ -662,13 +665,11 @@ void push_mulop::operator()(s3select* self, const char* a, const char* b) const
   }
 }
 
-void push_addsub_binop::operator()(s3select* self, const char* a, const char* b) const
+void push_addsub_binop::operator()(s3select* self, const char* a, [[maybe_unused]] const char* b) const
 {
-  base_statement* l = 0, *r = 0;
-
-  r = self->getAction()->exprQ.back();
+  base_statement* r = self->getAction()->exprQ.back();
   self->getAction()->exprQ.pop_back();
-  l = self->getAction()->exprQ.back();
+  base_statement* l = self->getAction()->exprQ.back();
   self->getAction()->exprQ.pop_back();
   addsub_operation::addsub_op_t o = self->getAction()->addsubQ.back();
   self->getAction()->addsubQ.pop_back();
@@ -676,13 +677,11 @@ void push_addsub_binop::operator()(s3select* self, const char* a, const char* b)
   self->getAction()->exprQ.push_back(as);
 }
 
-void push_mulldiv_binop::operator()(s3select* self, const char* a, const char* b) const
+void push_mulldiv_binop::operator()(s3select* self, const char* a, [[maybe_unused]] const char* b) const
 {
-  base_statement* vl = 0, *vr = 0;
-
-  vr = self->getAction()->exprQ.back();
+  base_statement* vr = self->getAction()->exprQ.back();
   self->getAction()->exprQ.pop_back();
-  vl = self->getAction()->exprQ.back();
+  base_statement* vl = self->getAction()->exprQ.back();
   self->getAction()->exprQ.pop_back();
   mulldiv_operation::muldiv_t o = self->getAction()->muldivQ.back();
   self->getAction()->muldivQ.pop_back();
@@ -734,27 +733,27 @@ void push_compare_operator::operator()(s3select* self, const char* a, const char
   std::string token(a, b);
   arithmetic_operand::cmp_t c = arithmetic_operand::cmp_t::NA;
 
-  if (token.compare("==") == 0)
+  if (token =="=="s)
   {
     c = arithmetic_operand::cmp_t::EQ;
   }
-  else if (token.compare("!=") == 0)
+  else if (token =="!="s)
   {
     c = arithmetic_operand::cmp_t::NE;
   }
-  else if (token.compare(">=") == 0)
+  else if (token ==">="s)
   {
     c = arithmetic_operand::cmp_t::GE;
   }
-  else if (token.compare("<=") == 0)
+  else if (token == "<="s)
   {
     c = arithmetic_operand::cmp_t::LE;
   }
-  else if (token.compare(">") == 0)
+  else if (token == ">"s)
   {
     c = arithmetic_operand::cmp_t::GT;
   }
-  else if (token.compare("<") == 0)
+  else if (token =="<")
   {
     c = arithmetic_operand::cmp_t::LT;
   }
@@ -771,11 +770,11 @@ void push_logical_operator::operator()(s3select* self, const char* a, const char
   std::string token(a, b);
   logical_operand::oplog_t l = logical_operand::oplog_t::NA;
 
-  if (token.compare("and") == 0)
+  if (token =="and")
   {
     l = logical_operand::oplog_t::AND;
   }
-  else if (token.compare("or") == 0)
+  else if (token =="or")
   {
     l = logical_operand::oplog_t::OR;
   }
@@ -807,17 +806,18 @@ void push_arithmetic_predicate::operator()(s3select* self, const char* a, const
 void push_logical_predicate::operator()(s3select* self, const char* a, const char* b) const
 {
   std::string token(a, b);
+  base_statement* tl = nullptr;
+  base_statement* tr = nullptr;
 
-  base_statement* tl = 0, *tr = 0;
   logical_operand::oplog_t oplog = self->getAction()->logical_compareQ.back();
   self->getAction()->logical_compareQ.pop_back();
 
-  if (self->getAction()->condQ.empty() == false)
+  if (!self->getAction()->condQ.empty())
   {
     tr = self->getAction()->condQ.back();
     self->getAction()->condQ.pop_back();
   }
-  if (self->getAction()->condQ.empty() == false)
+  if (!self->getAction()->condQ.empty())
   {
     tl = self->getAction()->condQ.back();
     self->getAction()->condQ.pop_back();
@@ -831,8 +831,9 @@ void push_logical_predicate::operator()(s3select* self, const char* a, const cha
 void push_negation::operator()(s3select* self, const char* a, const char* b) const
 {
   std::string token(a, b);
-  base_statement* pred;
-  if (self->getAction()->condQ.empty() == false)
+  base_statement* pred{nullptr};
+
+  if (!self->getAction()->condQ.empty())
   {
     pred = self->getAction()->condQ.back();
     self->getAction()->condQ.pop_back();
@@ -840,7 +841,7 @@ void push_negation::operator()(s3select* self, const char* a, const char* b) con
   //upon NOT operator, the logical and arithmetical operators are "tagged" to negate result.
   if (dynamic_cast<logical_operand*>(pred))
   {
-    logical_operand* f = S3SELECT_NEW(self, logical_operand, pred);
+    logical_operand* f = S3SELECT_NEW(self, logical_operand, pred); // todo: marked as "empty statemant"
     self->getAction()->condQ.push_back(f);
   }
   else if (dynamic_cast<__function*>(pred) || dynamic_cast<negate_function_operation*>(pred))
@@ -860,7 +861,7 @@ void push_column_pos::operator()(s3select* self, const char* a, const char* b) c
   std::string token(a, b);
   variable* v;
 
-  if (token.compare("*") == 0 || token.compare("* ") == 0) //TODO space should skip in boost::spirit
+  if (token == "*" || token == "* ") //TODO space should skip in boost::spirit
   {
     v = S3SELECT_NEW(self, variable, token, variable::var_t::STAR_OPERATION);
   }
@@ -892,7 +893,7 @@ void push_alias_projection::operator()(s3select* self, const char* a, const char
 
   //mapping alias name to base-statement
   bool res = self->getAction()->alias_map.insert_new_entry(alias_name, bs);
-  if (res == false)
+  if (!res)
   {
     throw base_s3select_exception(std::string("alias <") + alias_name + std::string("> is already been used in query"), base_s3select_exception::s3select_exp_en_t::FATAL);
   }
@@ -979,7 +980,8 @@ void push_like_predicate::operator()(s3select* self, const char* a, const char*
   if (!dynamic_cast<variable*>(expr))
   {
     throw base_s3select_exception("like expression must be a constant string", base_s3select_exception::s3select_exp_en_t::FATAL);
-  }else if( dynamic_cast<variable*>(expr)->m_var_type != variable::var_t::COL_VALUE)
+
+  } else if( dynamic_cast<variable*>(expr)->m_var_type != variable::var_t::COL_VALUE)
   {
     throw base_s3select_exception("like expression must be a constant string", base_s3select_exception::s3select_exp_en_t::FATAL);
   }
@@ -1087,16 +1089,16 @@ void push_data_type::operator()(s3select* self, const char* a, const char* b) co
 
   if(cast_operator("int"))
   {
-    self->getAction()->dataTypeQ.push_back("int");
+    self->getAction()->dataTypeQ.emplace_back("int");
   }else if(cast_operator("float"))
   {
-    self->getAction()->dataTypeQ.push_back("float");
+    self->getAction()->dataTypeQ.emplace_back("float");
   }else if(cast_operator("string"))
   {
-    self->getAction()->dataTypeQ.push_back("string");
+    self->getAction()->dataTypeQ.emplace_back("string");
   }else if(cast_operator("timestamp"))
   {
-    self->getAction()->dataTypeQ.push_back("timestamp");
+    self->getAction()->dataTypeQ.emplace_back("timestamp");
   }
 }
 
@@ -1114,7 +1116,7 @@ class base_s3object
   std::string m_obj_name;
 
 public:
-  base_s3object(scratch_area* m) : m_sa(m), m_obj_name("") {}
+  explicit base_s3object(scratch_area* m) : m_sa(m) {}
 
   void set(scratch_area* m)
   {
@@ -1122,7 +1124,7 @@ class base_s3object
     m_obj_name = "";
   }
 
-  virtual ~base_s3object() {}
+  virtual ~base_s3object() = default;
 };
 
 
@@ -1143,10 +1145,10 @@ class csv_object : public base_s3object
 
   } m_csv_defintion;
 
-  csv_object(s3select* s3_query) :
+  explicit csv_object(s3select* s3_query) :
     base_s3object(s3_query->get_scratch_area()),
     m_skip_last_line(false),
-    m_s3_select(0),
+    m_s3_select(nullptr),
     m_error_count(0),
     m_extract_csv_header_info(false),
     m_previous_line(false),
@@ -1160,7 +1162,7 @@ class csv_object : public base_s3object
   csv_object(s3select* s3_query, struct csv_defintions csv) :
     base_s3object(s3_query->get_scratch_area()),
     m_skip_last_line(false),
-    m_s3_select(0),
+    m_s3_select(nullptr),
     m_error_count(0),
     m_extract_csv_header_info(false),
     m_previous_line(false),
@@ -1173,9 +1175,9 @@ class csv_object : public base_s3object
   }
 
   csv_object():
-    base_s3object(0),
+    base_s3object(nullptr),
     m_skip_last_line(false),
-    m_s3_select(0),
+    m_s3_select(nullptr),
     m_error_count(0),
     m_extract_csv_header_info(false),
     m_previous_line(false),
@@ -1191,7 +1193,7 @@ class csv_object : public base_s3object
   bool m_aggr_flow = false; //TODO once per query
   bool m_is_to_aggregate;
   bool m_skip_last_line;
-  size_t m_stream_length;
+  //size_t m_stream_length;
   std::string m_error_description;
   char* m_stream;
   char* m_end_stream;
@@ -1263,17 +1265,17 @@ class csv_object : public base_s3object
     return m_error_description;
   }
 
-  virtual ~csv_object() {}
+  virtual ~csv_object() = default;
 
 public:
 
 
   int getMatchRow( std::string& result) //TODO virtual ? getResult
   {
-    int number_of_tokens = 0;
+    int number_of_tokens;
 
 
-    if (m_aggr_flow == true)
+    if (m_aggr_flow)
     {
       do
       {
@@ -1282,7 +1284,7 @@ class csv_object : public base_s3object
         if (number_of_tokens < 0) //end of stream
         {
           if (m_is_to_aggregate)
-            for (auto i : m_projections)
+            for (auto& i : m_projections)
             {
               i->set_last_call();
               result.append( i->eval().to_string() );
@@ -1305,13 +1307,13 @@ class csv_object : public base_s3object
         }
 
         if (!m_where_clause || m_where_clause->eval().i64() == true)
-          for (auto i : m_projections)
+          for (auto& i : m_projections)
           {
             i->eval();
           }
 
       }
-      while (1);
+      while (true);
     }
     else
     {
@@ -1334,7 +1336,7 @@ class csv_object : public base_s3object
       }
       while (m_where_clause && m_where_clause->eval().i64() == false);
 
-      for (auto i : m_projections)
+      for (auto& i : m_projections)
       {
         result.append( i->eval().to_string() );
         result.append(",");
@@ -1348,7 +1350,7 @@ class csv_object : public base_s3object
   int extract_csv_header_info()
   {
 
-    if (m_csv_defintion.ignore_header_info == true)
+    if (m_csv_defintion.ignore_header_info)
     {
       while(*m_stream && (*m_stream != m_csv_defintion.row_delimiter ))
       {
@@ -1356,7 +1358,7 @@ class csv_object : public base_s3object
       }
       m_stream++;
     }
-    else if(m_csv_defintion.use_header_info == true)
+    else if(m_csv_defintion.use_header_info)
     {
       size_t num_of_tokens = getNextRow();//TODO validate number of tokens
 
@@ -1424,7 +1426,7 @@ class csv_object : public base_s3object
       m_previous_line = false;
       m_skip_first_line = true;
 
-      status = run_s3select_on_object(result, merge_line.c_str(), merge_line.length(), false, false, false);
+      /* status = */ run_s3select_on_object(result, merge_line.c_str(), merge_line.length(), false, false, false);
     }
 
     if (csv_stream[stream_length - 1] != m_csv_defintion.row_delimiter)
@@ -1458,9 +1460,9 @@ class csv_object : public base_s3object
     m_is_to_aggregate = do_aggregate;
     m_skip_last_line = skip_last_line;
 
-    m_stream_length = stream_length;
+    // not used: m_stream_length = stream_length;
 
-    if(m_extract_csv_header_info == false)
+    if(!m_extract_csv_header_info)
     {
       extract_csv_header_info();
     }
@@ -1499,7 +1501,7 @@ class csv_object : public base_s3object
       }
 
     }
-    while (1);
+    while (true);
 
     return 0;
   }
diff --git a/include/s3select_functions.h b/include/s3select_functions.h
index 854c4970..b5bc3bd4 100644
--- a/include/s3select_functions.h
+++ b/include/s3select_functions.h
@@ -5,6 +5,8 @@
 #include "s3select_oper.h"
 #include <boost/algorithm/string.hpp>
 #include <regex>
+using namespace std::string_literals;
+
 
 #define BOOST_BIND_ACTION_PARAM( push_name ,param ) boost::bind( &push_name::operator(), g_ ## push_name , _1 ,_2, param)
 namespace s3selectEngine
@@ -103,9 +105,9 @@ class s3select_functions
 
 public:
 
-  base_function* create(std::string fn_name,bs_stmt_vec_t);
+  base_function* create(std::string_view fn_name, const bs_stmt_vec_t&);
 
-  s3select_functions():m_s3select_allocator(0)
+  s3select_functions():m_s3select_allocator(nullptr)
   {
   }
 
@@ -134,11 +136,11 @@ class __function : public base_statement
 private:
   bs_stmt_vec_t arguments;
   std::string name;
-  base_function* m_func_impl;
+  base_function* m_func_impl; // todo: separate the object types used when constructing the __function vs when m_func_impl should be fixed
   s3select_functions* m_s3select_functions;
   variable m_result;
 
-  void _resolve_name()
+  void _resolve_name()  // todo: separate the object types used when constructing the __function vs when m_func_impl should be fixed
   {
     if (m_func_impl)
     {
@@ -164,7 +166,7 @@ class __function : public base_statement
     return m_func_impl;
   }
 
-  virtual void traverse_and_apply(scratch_area* sa, projection_alias* pa)
+  void traverse_and_apply(scratch_area* sa, projection_alias* pa) override
   {
     m_scratch = sa;
     m_aliases = pa;
@@ -174,26 +176,25 @@ class __function : public base_statement
     }
   }
 
-  virtual bool is_aggregate() // TODO under semantic flow
+  bool is_aggregate() const override// TODO under semantic flow
   {
-    _resolve_name();
+    const_cast<__function*>(this)->_resolve_name();
 
     return m_func_impl->is_aggregate();
   }
 
-  virtual bool semantic()
+  bool semantic() override
   {
     return true;
   }
 
-  __function(const char* fname, s3select_functions* s3f) : name(fname), m_func_impl(0), m_s3select_functions(s3f) {}
+  __function(const char* fname, s3select_functions* s3f) : name(fname), m_func_impl(nullptr), m_s3select_functions(s3f) {}
 
-  virtual value& eval()
+  value& eval() override
   {
+    const_cast<__function*>(this)->_resolve_name();
 
-    _resolve_name();
-
-    if (is_last_call == false)
+    if (!is_last_call)
     {
       (*m_func_impl)(&arguments, &m_result);
     }
@@ -206,10 +207,9 @@ class __function : public base_statement
   }
 
 
-
-  virtual std::string  print(int ident)
+  std::string  print(int ident) override
   {
-    return std::string(0);
+    return std::string{};
   }
 
   void push_argument(base_statement* arg)
@@ -218,12 +218,12 @@ class __function : public base_statement
   }
 
 
-  bs_stmt_vec_t& get_arguments()
+  const bs_stmt_vec_t& get_arguments() const
   {
     return arguments;
   }
 
-  virtual ~__function() {}
+  virtual ~__function() = default;
 };
 
 
@@ -241,16 +241,16 @@ class __function : public base_statement
   }
 
 /*
-    s3-select function defintions
+    s3-select function definitions
 */
 struct _fn_add : public base_function
 {
 
   value var_result;
 
-  bool operator()(bs_stmt_vec_t* args, variable* result)
+  bool operator()(bs_stmt_vec_t* args, variable* result) override
   {
-    bs_stmt_vec_t::iterator iter = args->begin();
+    auto iter = args->begin();
     base_statement* x =  *iter;
     iter++;
     base_statement* y = *iter;
@@ -273,9 +273,9 @@ struct _fn_sum : public base_function
     aggregate = true;
   }
 
-  bool operator()(bs_stmt_vec_t* args, variable* result)
+  bool operator()(bs_stmt_vec_t* args, variable* result) override
   {
-    bs_stmt_vec_t::iterator iter = args->begin();
+    auto iter = args->begin();
     base_statement* x = *iter;
 
     try
@@ -294,7 +294,7 @@ struct _fn_sum : public base_function
     return true;
   }
 
-  virtual void get_aggregate_result(variable* result)
+  void get_aggregate_result(variable* result) override
   {
     *result = sum ;
   }
@@ -310,14 +310,14 @@ struct _fn_count : public base_function
     aggregate=true;
   }
 
-  bool operator()(bs_stmt_vec_t* args, variable* result)
+  bool operator()(bs_stmt_vec_t* args, variable* result) override
   {
     count += 1;
 
     return true;
   }
 
-  virtual void get_aggregate_result(variable* result)
+  void get_aggregate_result(variable* result) override
   {
     result->set_value(count);
   }
@@ -332,10 +332,9 @@ struct _fn_avg : public base_function
 
     _fn_avg() : sum(0) { aggregate = true; }
 
-    bool operator()(bs_stmt_vec_t* args, variable *result)
+    bool operator()(bs_stmt_vec_t* args, variable *result) override
     {
-        bs_stmt_vec_t::iterator iter = args->begin();
-        base_statement *x = *iter;
+        base_statement *x = *(args->begin());
 
         try
         {
@@ -350,7 +349,7 @@ struct _fn_avg : public base_function
         return true;
     }
 
-    virtual void get_aggregate_result(variable *result)
+     void get_aggregate_result(variable *result) override
     {
         if(count == 0) {
             throw base_s3select_exception("count cannot be zero!");
@@ -370,10 +369,9 @@ struct _fn_min : public base_function
     aggregate=true;
   }
 
-  bool operator()(bs_stmt_vec_t* args, variable* result)
+  bool operator()(bs_stmt_vec_t* args, variable* result) override
   {
-    bs_stmt_vec_t::iterator iter = args->begin();
-    base_statement* x =  *iter;
+    base_statement* x =  *(args->begin());
 
     if(min > x->eval())
     {
@@ -383,7 +381,7 @@ struct _fn_min : public base_function
     return true;
   }
 
-  virtual void get_aggregate_result(variable* result)
+   void get_aggregate_result(variable* result) override
   {
     *result = min;
   }
@@ -400,9 +398,9 @@ struct _fn_max : public base_function
     aggregate=true;
   }
 
-  bool operator()(bs_stmt_vec_t* args, variable* result)
+  bool operator()(bs_stmt_vec_t* args, variable* result) override
   {
-    bs_stmt_vec_t::iterator iter = args->begin();
+    auto iter = args->begin();
     base_statement* x =  *iter;
 
     if(max < x->eval())
@@ -413,7 +411,7 @@ struct _fn_max : public base_function
     return true;
   }
 
-  virtual void get_aggregate_result(variable* result)
+   void get_aggregate_result(variable* result) override
   {
     *result = max;
   }
@@ -426,7 +424,7 @@ struct _fn_to_int : public base_function
   value var_result;
   value func_arg;
 
-  bool operator()(bs_stmt_vec_t* args, variable* result)
+  bool operator()(bs_stmt_vec_t* args, variable* result) override
   {
     char* perr;
     int64_t i=0;
@@ -465,42 +463,44 @@ struct _fn_to_int : public base_function
 
 struct _fn_to_float : public base_function
 {
-
   value var_result;
-  value v_from;
 
-  bool operator()(bs_stmt_vec_t* args, variable* result)
+  bool operator()(bs_stmt_vec_t* args, variable* result) override
   {
     char* perr;
-    double d=0;
     value v = (*args->begin())->eval();
 
-    if (v.type == value::value_En_t::STRING)
+    switch (v.type) {
+
+    case value::value_En_t::STRING:
     {
-      errno = 0;
-      d = strtod(v.str(), &perr) ;  //TODO check error before constructor
-      if ((errno == ERANGE && (d == LONG_MAX || d == LONG_MIN)) || (errno != 0 && d == 0)) {
+      char* pend;
+      double d = strtod(v.str(), &pend);
+      if (errno == ERANGE) {
         throw base_s3select_exception("converted value would fall out of the range of the result type!");
-        return false;  
-    }
-    
-    if (*perr != '\0') {
-      throw base_s3select_exception("characters after float!");
-      return false;
-    }
-  }
-    else if (v.type == value::value_En_t::FLOAT)
-    {
-      d = v.dbl();
+      }
+      if (pend == v.str()) {
+        // no number found
+        throw base_s3select_exception("text cannot be converted to a number");
+      }
+      if (*pend) {
+        throw base_s3select_exception("extra characters after the number");
+      }
+
+      var_result = d;
+      break;
     }
-    else
-    {
-      d = v.i64();
+
+    case value::value_En_t::FLOAT:
+      var_result = v.dbl();
+      break;
+
+    default:
+      var_result = v.i64();
+      break;
     }
 
-    var_result = d;
     *result = var_result;
-
     return true;
   }
 
@@ -528,7 +528,7 @@ struct _fn_to_timestamp : public base_function
   value v_str;
 
 
-  bool datetime_validation()
+  [[nodiscard]] bool datetime_validation() const
   {
     //TODO temporary , should check for leap year
 
@@ -560,14 +560,14 @@ struct _fn_to_timestamp : public base_function
     return true;
   }
 
-  bool operator()(bs_stmt_vec_t* args, variable* result)
+  bool operator()(bs_stmt_vec_t* args, variable* result) override
   {
 
     hr = 0;
     mn = 0;
     sc = 0;
 
-    bs_stmt_vec_t::iterator iter = args->begin();
+    auto iter = args->begin();
     int args_size = args->size();
 
     if (args_size != 1)
@@ -586,7 +586,7 @@ struct _fn_to_timestamp : public base_function
 
     bsc::parse_info<> info_dig = bsc::parse(v_str.str(), d_yyyymmdd_dig >> *(separator) >> d_time_dig);
 
-    if(datetime_validation()==false or !info_dig.full)
+    if(!datetime_validation() or !info_dig.full)
     {
       throw base_s3select_exception("input date-time is illegal");
     }
@@ -608,9 +608,9 @@ struct _fn_extact_from_timestamp : public base_function
 
   value val_date_part;
 
-  bool operator()(bs_stmt_vec_t* args, variable* result)
+  bool operator()(bs_stmt_vec_t* args, variable* result) override
   {
-    bs_stmt_vec_t::iterator iter = args->begin();
+    auto iter = args->begin();
     int args_size = args->size();
 
     if (args_size < 2)
@@ -622,7 +622,7 @@ struct _fn_extact_from_timestamp : public base_function
 
     val_date_part = date_part->eval();//TODO could be done once?
 
-    if(val_date_part.is_string()== false)
+    if(!val_date_part.is_string())
     {
       throw base_s3select_exception("first parameter should be string");
     }
@@ -631,7 +631,7 @@ struct _fn_extact_from_timestamp : public base_function
 
     base_statement* ts = *iter;
 
-    if(ts->eval().is_timestamp()== false)
+    if(!ts->eval().is_timestamp())
     {
       throw base_s3select_exception("second parameter is not timestamp");
     }
@@ -671,9 +671,9 @@ struct _fn_diff_timestamp : public base_function
   value val_dt1;
   value val_dt2;
 
-  bool operator()(bs_stmt_vec_t* args, variable* result)
+  bool operator()(bs_stmt_vec_t* args, variable* result) override
   {
-    bs_stmt_vec_t::iterator iter = args->begin();
+    auto iter = args->begin();
     int args_size = args->size();
 
     if (args_size < 3)
@@ -688,7 +688,7 @@ struct _fn_diff_timestamp : public base_function
     iter++;
     base_statement* dt1_param = *iter;
     val_dt1 = dt1_param->eval();
-    if (val_dt1.is_timestamp() == false)
+    if (!val_dt1.is_timestamp())
     {
       throw base_s3select_exception("second parameter should be timestamp");
     }
@@ -696,7 +696,7 @@ struct _fn_diff_timestamp : public base_function
     iter++;
     base_statement* dt2_param = *iter;
     val_dt2 = dt2_param->eval();
-    if (val_dt2.is_timestamp() == false)
+    if (!val_dt2.is_timestamp())
     {
       throw base_s3select_exception("third parameter should be timestamp");
     }
@@ -743,9 +743,9 @@ struct _fn_add_to_timestamp : public base_function
   value val_quantity;
   value val_timestamp;
 
-  bool operator()(bs_stmt_vec_t* args, variable* result)
+  bool operator()(bs_stmt_vec_t* args, variable* result) override
   {
-    bs_stmt_vec_t::iterator iter = args->begin();
+    auto iter = args->begin();
     int args_size = args->size();
 
     if (args_size < 3)
@@ -756,7 +756,7 @@ struct _fn_add_to_timestamp : public base_function
     base_statement* date_part = *iter;
     val_date_part = date_part->eval();//TODO could be done once?
 
-    if(val_date_part.is_string()== false)
+    if(!val_date_part.is_string())
     {
       throw base_s3select_exception("first parameter should be string");
     }
@@ -765,7 +765,7 @@ struct _fn_add_to_timestamp : public base_function
     base_statement* quan = *iter;
     val_quantity = quan->eval();
 
-    if (val_quantity.is_number() == false)
+    if (!val_quantity.is_number())
     {
       throw base_s3select_exception("second parameter should be number");  //TODO what about double?
     }
@@ -774,7 +774,7 @@ struct _fn_add_to_timestamp : public base_function
     base_statement* ts = *iter;
     val_timestamp = ts->eval();
 
-    if(val_timestamp.is_timestamp() == false)
+    if(!val_timestamp.is_timestamp())
     {
       throw base_s3select_exception("third parameter should be time-stamp");
     }
@@ -811,7 +811,7 @@ struct _fn_utcnow : public base_function
 
   boost::posix_time::ptime now_ptime;
 
-  bool operator()(bs_stmt_vec_t* args, variable* result)
+  bool operator()(bs_stmt_vec_t* args, variable* result) override
   {
     int args_size = args->size();
 
@@ -832,7 +832,7 @@ struct _fn_between : public base_function
 
   value res;
   
-  bool operator()(bs_stmt_vec_t* args, variable* result)
+  bool operator()(bs_stmt_vec_t* args, variable* result) override
   {
     int args_size = args->size();
 
@@ -842,7 +842,7 @@ struct _fn_between : public base_function
       throw base_s3select_exception("between operates on 3 expressions");//TODO FATAL
     }
 
-    bs_stmt_vec_t::iterator iter = args->begin();
+    auto iter = args->begin();
 
     base_statement* second_expr = *iter;
     iter++;    
@@ -871,7 +871,7 @@ static char s3select_ver[10]="41.a";
 struct _fn_version : public base_function
 {
   value val; //TODO use git to generate sha1
-  bool operator()(bs_stmt_vec_t* args, variable* result)
+  bool operator()(bs_stmt_vec_t* args, variable* result) override
   {
     val = &s3select_ver[0];
     *result = val;
@@ -884,9 +884,9 @@ struct _fn_isnull : public base_function
 
   value res;
   
-  bool operator()(bs_stmt_vec_t* args, variable* result)
+  bool operator()(bs_stmt_vec_t* args, variable* result) override
   {
-    bs_stmt_vec_t::iterator iter = args->begin();
+    auto iter = args->begin();
     base_statement* expr = *iter;
     value expr_val = expr->eval();
     if ( expr_val.is_null()) {
@@ -903,7 +903,7 @@ struct _fn_in : public base_function
 
   value res;
 
-  bool operator()(bs_stmt_vec_t *args, variable *result)
+  bool operator()(bs_stmt_vec_t *args, variable *result) override
   {
     int args_size = args->size()-1;
     base_statement *main_expr = (*args)[args_size];
@@ -941,7 +941,7 @@ struct _fn_like : public base_function
     compiled_regex = std::regex(string_value);
   } 
 
-  void transform(std::string& s)
+  static void transform(std::string& s)
   {
     std::string::size_type i = 0;
     while (!s.empty())
@@ -988,9 +988,9 @@ struct _fn_like : public base_function
     }
   }
   
-  bool operator()(bs_stmt_vec_t* args, variable* result)
+  bool operator()(bs_stmt_vec_t* args, variable* result) override
   {
-    bs_stmt_vec_t::iterator iter = args->begin();
+    auto iter = args->begin();
     iter++;
     base_statement* main_expr = *iter;
     value main_expr_val = main_expr->eval();
@@ -1015,9 +1015,9 @@ struct _fn_substr : public base_function
   value v_from;
   value v_to;
 
-  bool operator()(bs_stmt_vec_t* args, variable* result)
+  bool operator()(bs_stmt_vec_t* args, variable* result) override
   {
-    bs_stmt_vec_t::iterator iter = args->begin();
+    auto iter = args->begin();
     int args_size = args->size();
 
 
@@ -1117,9 +1117,9 @@ struct _fn_charlength : public base_function {
 
     value v_str;
  
-    bool operator()(bs_stmt_vec_t* args, variable* result)
+    bool operator()(bs_stmt_vec_t* args, variable* result) override
     {
-        bs_stmt_vec_t::iterator iter = args->begin();
+        auto iter = args->begin();
         base_statement* str =  *iter;
         v_str = str->eval();
         if(v_str.type != value::value_En_t::STRING) {
@@ -1137,9 +1137,9 @@ struct _fn_lower : public base_function {
     std::string buff;
     value v_str;
 
-    bool operator()(bs_stmt_vec_t* args, variable* result)
+    bool operator()(bs_stmt_vec_t* args, variable* result) override
     {
-        bs_stmt_vec_t::iterator iter = args->begin();
+        auto iter = args->begin();
         base_statement* str = *iter;
         v_str = str->eval();
         if(v_str.type != value::value_En_t::STRING) {
@@ -1158,9 +1158,9 @@ struct _fn_upper : public base_function {
     std::string buff;
     value v_str;
 
-    bool operator()(bs_stmt_vec_t* args, variable* result)
+    bool operator()(bs_stmt_vec_t* args, variable* result) override
     {
-        bs_stmt_vec_t::iterator iter = args->begin();
+        auto iter = args->begin();
         base_statement* str = *iter;
         v_str = str->eval();
         if(v_str.type != value::value_En_t::STRING) {
@@ -1179,9 +1179,9 @@ struct _fn_nullif : public base_function {
     value x;
     value y;
 
-    bool operator()(bs_stmt_vec_t* args, variable* result)
+    bool operator()(bs_stmt_vec_t* args, variable* result) override
     {
-        bs_stmt_vec_t::iterator iter = args->begin();
+        auto iter = args->begin();
 
         int args_size = args->size();
         if (args_size != 2)
@@ -1220,11 +1220,11 @@ struct _fn_nullif : public base_function {
 
 struct _fn_when_than : public base_function {
 
-  value when_value,than_value;
+  value when_value;
 
-  bool operator()(bs_stmt_vec_t* args, variable* result)
+  bool operator()(bs_stmt_vec_t* args, variable* result) override
   {
-    bs_stmt_vec_t::iterator iter = args->begin();
+    auto iter = args->begin();
 
     base_statement* than_expr = *iter;
     iter ++;
@@ -1249,7 +1249,7 @@ struct _fn_case_when_else : public base_function {
 
   value when_than_value;
 
-  bool operator()(bs_stmt_vec_t* args, variable* result)
+  bool operator()(bs_stmt_vec_t* args, variable* result) override
   {
     base_statement* else_expr = *(args->begin());
 
@@ -1277,9 +1277,9 @@ struct _fn_coalesce : public base_function
 
   value res;
 
-  bool operator()(bs_stmt_vec_t* args, variable* result)
+  bool operator()(bs_stmt_vec_t* args, variable* result) override
   {
-    bs_stmt_vec_t::iterator iter_begin = args->begin();
+    auto iter_begin = args->begin();
     int args_size = args->size();
     while (args_size >= 1)
     {
@@ -1302,9 +1302,9 @@ struct _fn_string : public base_function
 
   value res;
 
-  bool operator()(bs_stmt_vec_t* args, variable* result)
+  bool operator()(bs_stmt_vec_t* args, variable* result) override
   {
-    bs_stmt_vec_t::iterator iter = args->begin();
+    auto iter = args->begin();
 
     base_statement* expr = *iter;
     value expr_val = expr->eval();
@@ -1313,14 +1313,14 @@ struct _fn_string : public base_function
   }
 };
 
-base_function* s3select_functions::create(std::string fn_name,bs_stmt_vec_t arguments)
+base_function* s3select_functions::create(std::string_view fn_name, const bs_stmt_vec_t& arguments)
 {
-  const FunctionLibrary::const_iterator iter = m_functions_library.find(fn_name);
+  const auto iter = m_functions_library.find(fn_name.data());
 
   if (iter == m_functions_library.end())
   {
     std::string msg;
-    msg = fn_name + " " + " function not found";
+    msg = std::string{fn_name} + " function not found"s;
     throw base_s3select_exception(msg, base_s3select_exception::s3select_exp_en_t::FATAL);
   }
 
@@ -1439,9 +1439,9 @@ base_function* s3select_functions::create(std::string fn_name,bs_stmt_vec_t argu
   }
 }
 
-bool base_statement::is_function()
+bool base_statement::is_function() const
 {
-  if (dynamic_cast<__function*>(this))
+  if (dynamic_cast<__function*>(const_cast<base_statement*>(this)))
   {
     return true;
   }
@@ -1451,27 +1451,27 @@ bool base_statement::is_function()
   }
 }
 
-bool base_statement::is_aggregate_exist_in_expression(base_statement* e) //TODO obsolete ?
+bool base_statement::is_aggregate_exist_in_expression() const //TODO obsolete ?
 {
-  if (e->is_aggregate())
+  if (is_aggregate())
   {
     return true;
   }
 
-  if (e->left() && e->left()->is_aggregate_exist_in_expression(e->left()))
+  if (left() && left()->is_aggregate_exist_in_expression())
   {
     return true;
   }
 
-  if (e->right() && e->right()->is_aggregate_exist_in_expression(e->right()))
+  if (right() && right()->is_aggregate_exist_in_expression())
   {
     return true;
   }
 
-  if (e->is_function())
+  if (is_function())
   {
-    for (auto i : dynamic_cast<__function*>(e)->get_arguments())
-      if (e->is_aggregate_exist_in_expression(i))
+    for (const auto& i : dynamic_cast<__function*>(const_cast<base_statement*>(this))->get_arguments())
+      if (i->is_aggregate_exist_in_expression())
       {
         return true;
       }
@@ -1480,64 +1480,63 @@ bool base_statement::is_aggregate_exist_in_expression(base_statement* e) //TODO
   return false;
 }
 
-base_statement* base_statement::get_aggregate()
+const base_statement* base_statement::get_aggregate() const
 {
   //search for aggregation function in AST
-  base_statement* res = 0;
+  const base_statement* res;
 
   if (is_aggregate())
   {
     return this;
   }
 
-  if (left() && (res=left()->get_aggregate())!=0)
+  if (left() && (res=left()->get_aggregate())!=nullptr)
   {
     return res;
   }
 
-  if (right() && (res=right()->get_aggregate())!=0)
+  if (right() && (res=right()->get_aggregate())!=nullptr)
   {
     return res;
   }
 
   if (is_function())
   {
-    for (auto i : dynamic_cast<__function*>(this)->get_arguments())
+    for (const auto& i : dynamic_cast<__function*>(const_cast<base_statement*>(this))->get_arguments())
     {
-      base_statement* b=i->get_aggregate();
-      if (b)
+      if (auto b = i->get_aggregate(); b)
       {
         return b;
       }
     }
   }
-  return 0;
+  return nullptr;
 }
 
-bool base_statement::is_nested_aggregate(base_statement* e)
+bool base_statement::is_nested_aggregate() const
 {
   //validate for non nested calls for aggregation function, i.e. sum ( min ( ))
-  if (e->is_aggregate())
+  if (is_aggregate())
   {
-    if (e->left())
+    if (left())
     {
-      if (e->left()->is_aggregate_exist_in_expression(e->left()))
+      if (left()->is_aggregate_exist_in_expression())
       {
         return true;
       }
     }
-    else if (e->right())
+    else if (right())
     {
-      if (e->right()->is_aggregate_exist_in_expression(e->right()))
+      if (right()->is_aggregate_exist_in_expression())
       {
         return true;
       }
     }
-    else if (e->is_function())
+    else if (is_function())
     {
-      for (auto i : dynamic_cast<__function*>(e)->get_arguments())
+      for (const auto& i : dynamic_cast<__function*>(const_cast<base_statement*>(this))->get_arguments())
       {
-        if (i->is_aggregate_exist_in_expression(i))
+        if (i->is_aggregate_exist_in_expression())
         {
           return true;
         }
@@ -1549,7 +1548,7 @@ bool base_statement::is_nested_aggregate(base_statement* e)
 }
 
 // select sum(c2) ... + c1 ... is not allowed. a binary operation with scalar is OK. i.e. select sum() + 1
-bool base_statement::is_binop_aggregate_and_column(base_statement* skip_expression)
+bool base_statement::is_binop_aggregate_and_column(const base_statement* skip_expression) const
 {
   if (left() && left() != skip_expression) //can traverse to left
   {
@@ -1557,7 +1556,7 @@ bool base_statement::is_binop_aggregate_and_column(base_statement* skip_expressi
     {
       return true;
     }
-    else if (left()->is_binop_aggregate_and_column(skip_expression) == true)
+    else if (left()->is_binop_aggregate_and_column(skip_expression))
     {
       return true;
     }
@@ -1569,7 +1568,7 @@ bool base_statement::is_binop_aggregate_and_column(base_statement* skip_expressi
     {
       return true;
     }
-    else if (right()->is_binop_aggregate_and_column(skip_expression) == true)
+    else if (right()->is_binop_aggregate_and_column(skip_expression))
     {
       return true;
     }
@@ -1578,7 +1577,7 @@ bool base_statement::is_binop_aggregate_and_column(base_statement* skip_expressi
   if (this != skip_expression && is_function())
   {
 
-    __function* f = (dynamic_cast<__function*>(this));
+    const __function* f = (dynamic_cast<const __function*>(this));
     bs_stmt_vec_t l = f->get_arguments();
     for (auto i : l)
     {
@@ -1586,7 +1585,7 @@ bool base_statement::is_binop_aggregate_and_column(base_statement* skip_expressi
       {
         return true;
       }
-      if (i->is_binop_aggregate_and_column(skip_expression) == true)
+      if (i->is_binop_aggregate_and_column(skip_expression))
       {
         return true;
       }
diff --git a/include/s3select_oper.h b/include/s3select_oper.h
index d7fe49dc..9e8e35e9 100644
--- a/include/s3select_oper.h
+++ b/include/s3select_oper.h
@@ -7,8 +7,8 @@
 #include <map>
 #include <vector>
 #include <algorithm>
-#include <string.h>
-#include <math.h>
+#include <cstring>
+#include <cmath>
 
 #include <boost/lexical_cast.hpp>
 #include <boost/date_time/posix_time/posix_time.hpp>
@@ -31,7 +31,7 @@ class ChunkAllocator : public std::allocator<T>
 public:
   typedef size_t size_type;
   typedef T* pointer;
-  typedef const T* const_pointer;
+  //typedef const T* const_pointer;
   size_t buffer_capacity;
   char* buffer_ptr;
 
@@ -49,7 +49,7 @@ class ChunkAllocator : public std::allocator<T>
   {
     // allocate storage for _Count elements of type T
 
-    pointer res = (pointer)(buffer_ptr + buffer_capacity);
+    auto res = (pointer)(buffer_ptr + buffer_capacity);
 
     buffer_capacity+= sizeof(T) * num_of_element;
     
@@ -65,18 +65,18 @@ class ChunkAllocator : public std::allocator<T>
   }
 
   //==================================
-  inline pointer allocate(size_type n, const void* hint = 0)
+  inline pointer allocate(size_type n, const void* hint = 0) // todo remove as both hides non-virtual and unused
   {
     return (_Allocate(n, (pointer)0));
   }
 
   //==================================
-  inline void deallocate(pointer p, size_type n)
+  inline void deallocate(pointer p, size_type n) // todo remove as both hides non-virtual and unused
   {
   }
 
   //==================================
-  ChunkAllocator() throw() : std::allocator<T>()
+  ChunkAllocator() noexcept : std::allocator<T>()
   {
     // alloc from main-buffer
     buffer_capacity = 0;
@@ -85,7 +85,7 @@ class ChunkAllocator : public std::allocator<T>
   }
 
   //==================================
-  ChunkAllocator(const ChunkAllocator& other) throw() : std::allocator<T>(other)
+  ChunkAllocator(const ChunkAllocator& other) noexcept : std::allocator<T>(other)
   {
     // copy const
     buffer_capacity = 0;
@@ -93,7 +93,7 @@ class ChunkAllocator : public std::allocator<T>
   }
 
   //==================================
-  ~ChunkAllocator() throw()
+  ~ChunkAllocator() noexcept
   {
     //do nothing
   }
@@ -121,7 +121,7 @@ class base_s3select_exception
 
 public:
   std::string _msg;
-  base_s3select_exception(const char* n) : m_severity(s3select_exp_en_t::NONE)
+  explicit base_s3select_exception(const char* n) : m_severity(s3select_exp_en_t::NONE)
   {
     _msg.assign(n);
   }
@@ -144,7 +144,7 @@ class base_s3select_exception
     return m_severity;
   }
 
-  virtual ~base_s3select_exception() {}
+  virtual ~base_s3select_exception() = default;
 };
 
 
@@ -225,7 +225,7 @@ class scratch_area
 
   void set_column_pos(const char* n, int pos)//TODO use std::string
   {
-    m_column_name_pos.push_back( std::pair<const char*, int>(n, pos));
+    m_column_name_pos.emplace_back(std::pair<const char*, int>(n, pos));
   }
 
   void update(const std::vector<char*>& tokens, size_t num_of_tokens)
@@ -259,7 +259,7 @@ class scratch_area
     return m_columns[column_pos];
   }
 
-  int get_num_of_columns()
+  int get_num_of_columns() const
   {
     return m_upper_bound;
   }
@@ -272,7 +272,7 @@ class s3select_reserved_word
   enum class reserve_word_en_t
   {
     NA,
-    S3S_NULL,//TODO check AWS defintions for reserve words, its a long list , what about functions-names? 
+    S3S_NULL,//TODO check AWS definitions for reserve words, its a long list , what about functions-names?
     S3S_NAN 
   } ;
 
@@ -291,7 +291,7 @@ class s3select_reserved_word
 
   reserve_word_en_t get_reserved_word(std::string & token)
   {
-    if (is_reserved_word(token)==true)
+    if (is_reserved_word(token))
     {
       return m_reserved_words.find(token)->second;
     }
@@ -322,11 +322,11 @@ class projection_alias
   {
     //purpose: only unique alias names.
 
-    for(auto alias: alias_map)
+    for(const auto& alias: alias_map)
     {
-      if(alias.first.compare(alias_name) == 0)
+      if(alias.first == alias_name)
       {
-        return false;  //alias name already exist
+        return false;  //alias name already exists
       }
 
     }
@@ -336,11 +336,11 @@ class projection_alias
     return true;
   }
 
-  base_statement* search_alias(std::string alias_name)
+  base_statement* search_alias(const std::string&& alias_name) const
   {
-    for(auto alias: alias_map)
+    for(const auto& alias: alias_map)
     {
-      if(alias.first.compare(alias_name) == 0)
+      if(alias.first == alias_name)
       {
         return alias.second;  //refernce to execution node
       }
@@ -414,7 +414,7 @@ class value
 {
 
 public:
-  typedef union
+  typedef union  // consider std::variant
   {
     int64_t num;
     char* str;//TODO consider string_view
@@ -914,11 +914,11 @@ class base_statement
 public:
   base_statement():m_scratch(0), is_last_call(false), m_is_cache_result(false), m_projection_alias(0), m_eval_stack_depth(0) {}
   virtual value& eval() =0;
-  virtual base_statement* left()
+  virtual base_statement* left() const
   {
     return 0;
   }
-  virtual base_statement* right()
+  virtual base_statement* right() const
   {
     return 0;
   }
@@ -939,20 +939,24 @@ class base_statement
     }
   }
 
-  virtual bool is_aggregate()
+  virtual bool is_aggregate() const
   {
     return false;
   }
-  virtual bool is_column()
+  virtual bool is_column() const
   {
     return false;
   }
 
-  bool is_function();
-  bool is_aggregate_exist_in_expression(base_statement* e);//TODO obsolete ?
-  base_statement* get_aggregate();
-  bool is_nested_aggregate(base_statement* e);
-  bool is_binop_aggregate_and_column(base_statement* skip);
+  bool is_function() const;
+
+  //bool is_aggregate_exist_in_expression(const base_statement* e) const;//TODO obsolete ?
+  bool is_aggregate_exist_in_expression() const;//TODO obsolete ?
+
+  const base_statement* get_aggregate() const;
+  //bool is_nested_aggregate(base_statement* e) const;
+  bool is_nested_aggregate() const;
+  bool is_binop_aggregate_and_column(const base_statement* skip) const;
 
   virtual void set_last_call()
   {
@@ -967,7 +971,7 @@ class base_statement
     }
   }
 
-  bool is_set_last_call()
+  bool is_set_last_call() const
   {
     return is_last_call;
   }
@@ -977,7 +981,7 @@ class base_statement
     m_is_cache_result = false;
   }
 
-  bool is_result_cached()
+  bool is_result_cached() const
   {
     return m_is_cache_result == true;
   }
@@ -1186,7 +1190,7 @@ class variable : public base_statement
     return var_value;
   }
 
-  virtual value& eval()
+   value& eval() override
   {
     if (m_var_type == var_t::COL_VALUE)
     {
@@ -1229,7 +1233,7 @@ class variable : public base_statement
         throw base_s3select_exception("number of calls exceed maximum size, probably a cyclic reference to alias", base_s3select_exception::s3select_exp_en_t::FATAL);
       }
 
-      if (m_projection_alias->is_result_cached() == false)
+      if (!m_projection_alias->is_result_cached())
       {
         var_value = m_projection_alias->eval();
         m_projection_alias->set_result_cache(var_value);
@@ -1249,14 +1253,14 @@ class variable : public base_statement
     return var_value;
   }
 
-  virtual std::string print(int ident)
+   std::string print(int ident) override
   {
     //std::string out = std::string(ident,' ') + std::string("var:") + std::to_string(var_value.__val.num);
     //return out;
     return std::string("#");//TBD
   }
 
-  virtual bool semantic()
+   bool semantic() override
   {
     return false;
   }
@@ -1280,28 +1284,28 @@ class arithmetic_operand : public base_statement
   
 public:
 
-  virtual bool semantic()
+   bool semantic() override
   {
     return true;
   }
 
-  virtual base_statement* left()
+   base_statement* left() const override
   {
     return l;
   }
-  virtual base_statement* right()
+   base_statement* right() const override
   {
     return r;
   }
 
-  virtual std::string print(int ident)
+   std::string print(int ident) override
   {
     //std::string out = std::string(ident,' ') + "compare:" += std::to_string(_cmp) + "\n" + l->print(ident-5) +r->print(ident+5);
     //return out;
     return std::string("#");//TBD
   }
 
-  virtual value& eval()
+   value& eval() override
   {
 
     switch (_cmp)
@@ -1338,7 +1342,7 @@ class arithmetic_operand : public base_statement
 
   arithmetic_operand(base_statement* _l, cmp_t c, base_statement* _r):l(_l), r(_r), _cmp(c),negation_result(false) {}
   
-  arithmetic_operand(base_statement* p)//NOT operator 
+  explicit arithmetic_operand(base_statement* p)//NOT operator
   {
     l = dynamic_cast<arithmetic_operand*>(p)->l;
     r = dynamic_cast<arithmetic_operand*>(p)->r;
@@ -1347,7 +1351,7 @@ class arithmetic_operand : public base_statement
     negation_result = ! dynamic_cast<arithmetic_operand*>(p)->negation_result;
   }
 
-  virtual ~arithmetic_operand() {}
+  virtual ~arithmetic_operand() = default;
 };
 
 class logical_operand : public base_statement
@@ -1367,23 +1371,23 @@ class logical_operand : public base_statement
 
 public:
 
-  virtual base_statement* left()
+  base_statement* left() const override
   {
     return l;
   }
-  virtual base_statement* right()
+  base_statement* right() const override
   {
     return r;
   }
 
-  virtual bool semantic()
+  bool semantic() override
   {
     return true;
   }
 
   logical_operand(base_statement* _l, oplog_t _o, base_statement* _r):l(_l), r(_r), _oplog(_o),negation_result(false) {}
 
-  logical_operand(base_statement * p)//NOT operator
+  explicit logical_operand(base_statement * p)//NOT operator
   {
     l = dynamic_cast<logical_operand*>(p)->l;
     r = dynamic_cast<logical_operand*>(p)->r;
@@ -1394,13 +1398,14 @@ class logical_operand : public base_statement
 
   virtual ~logical_operand() {}
 
-  virtual std::string print(int ident)
+   std::string print(int ident) override
   {
     //std::string out = std::string(ident, ' ') + "logical_operand:" += std::to_string(_oplog) + "\n" + l->print(ident - 5) + r->print(ident + 5);
     //return out;
     return std::string("#");//TBD
   }
-  virtual value& eval()
+
+  value& eval() override
   {
     bool res;
     if (!l || !r)
@@ -1412,7 +1417,7 @@ class logical_operand : public base_statement
     {
       if (a.i64() == false)
       {
-        res = false ^ negation_result;
+        res = negation_result;
         return var_value = res;
       } 
       value b = r->eval();
@@ -1426,7 +1431,7 @@ class logical_operand : public base_statement
     {
       if (a.i64() == true)
       {
-        res = true ^ negation_result;
+        res = !negation_result;
         return var_value = res;
       }
       value b = r->eval();
@@ -1457,28 +1462,28 @@ class mulldiv_operation : public base_statement
 
 public:
 
-  virtual base_statement* left()
+   base_statement* left() const override
   {
     return l;
   }
-  virtual base_statement* right()
+   base_statement* right() const override
   {
     return r;
   }
 
-  virtual bool semantic()
+   bool semantic()  override
   {
     return true;
   }
 
-  virtual std::string print(int ident)
+   std::string print(int ident) override
   {
     //std::string out = std::string(ident, ' ') + "mulldiv_operation:" += std::to_string(_mulldiv) + "\n" + l->print(ident - 5) + r->print(ident + 5);
     //return out;
     return std::string("#");//TBD
   }
 
-  virtual value& eval()
+   value& eval() override
   {
     switch (_mulldiv)
     {
@@ -1510,7 +1515,7 @@ class mulldiv_operation : public base_statement
 
   mulldiv_operation(base_statement* _l, muldiv_t c, base_statement* _r):l(_l), r(_r), _mulldiv(c) {}
 
-  virtual ~mulldiv_operation() {}
+  virtual ~mulldiv_operation() = default;
 };
 
 class addsub_operation : public base_statement
@@ -1530,31 +1535,31 @@ class addsub_operation : public base_statement
 
 public:
 
-  virtual base_statement* left()
+   base_statement* left() const override
   {
     return l;
   }
-  virtual base_statement* right()
+   base_statement* right() const override
   {
     return r;
   }
 
-  virtual bool semantic()
+   bool semantic() override
   {
     return true;
   }
 
   addsub_operation(base_statement* _l, addsub_op_t _o, base_statement* _r):l(_l), r(_r), _op(_o) {}
 
-  virtual ~addsub_operation() {}
+  virtual ~addsub_operation() = default;
 
-  virtual std::string print(int ident)
+   std::string print(int ident) override
   {
     //std::string out = std::string(ident, ' ') + "addsub_operation:" += std::to_string(_op) + "\n" + l->print(ident - 5) + r->print(ident + 5);
     return std::string("#");//TBD
   }
 
-  virtual value& eval()
+   value& eval() override
   {
     if (_op == addsub_op_t::NA) // -num , +num , unary-operation on number
     {
@@ -1591,24 +1596,24 @@ class negate_function_operation : public base_statement
   
   public:
 
-  negate_function_operation(base_statement *f):function_to_negate(f){}
+  explicit negate_function_operation(base_statement *f):function_to_negate(f){}
 
-  virtual std::string print(int ident)
+   std::string print(int ident) override
   {
     return std::string("#");//TBD
   }
 
-  virtual bool semantic()
+   bool semantic() override
   {
     return true;
   }
 
-  virtual base_statement* left()
+  base_statement* left() const override
   {
     return function_to_negate;
   }
 
-  virtual value& eval()
+   value& eval() override
   {
     res = function_to_negate->eval();
 
@@ -1640,13 +1645,13 @@ class base_function
   // validate semantic on creation instead on run-time
   virtual bool operator()(bs_stmt_vec_t* args, variable* result) = 0;
   base_function() : aggregate(false) {}
-  bool is_aggregate()
+  bool is_aggregate() const
   {
-    return aggregate == true;
+    return aggregate;
   }
   virtual void get_aggregate_result(variable*) {}
 
-  virtual ~base_function() {}
+  virtual ~base_function() = default;
   
   virtual void dtor()
   {//release function-body implementation 
@@ -1655,6 +1660,6 @@ class base_function
 };
 
 
-};//namespace
+}//namespace
 
 #endif
diff --git a/test/s3select_test.cpp b/test/s3select_test.cpp
index e15c91b9..94950d97 100644
--- a/test/s3select_test.cpp
+++ b/test/s3select_test.cpp
@@ -1,7 +1,7 @@
 #include "s3select.h"
 #include "gtest/gtest.h"
 #include <string>
-#include "boost/date_time/gregorian/gregorian.hpp"
+//#include "boost/date_time/gregorian/gregorian.hpp"
 #include "boost/date_time/posix_time/posix_time.hpp"
 
 using namespace s3selectEngine;
@@ -95,7 +95,7 @@ class gen_expr
 
   std::string generate()
   {
-    std::string exp = "";
+    std::string exp;
     open = 0;
 
     for (int i = 0; i < 10; i++)
@@ -142,7 +142,7 @@ TEST(TestS3SElect, s3select_vs_C)
     std::string exp = g.generate();
     std::string c_result = run_expression_in_C_prog( exp.c_str() );
 
-    char* err=0;
+    char* err;
     double  c_dbl_res = strtod(c_result.c_str(), &err);
 
     std::string input_query = "select " + exp + " from stdin;" ;
@@ -160,7 +160,7 @@ TEST(TestS3SElect, s3select_vs_C)
 TEST(TestS3SElect, ParseQuery)
 {
   //TODO syntax issues ?
-  //TODO error messeges ?
+  //TODO error messages ?
 
   s3select s3select_syntax;