Fix some of the hints from the 1on1 session on the refactoring the code

bin/serene.cpp

@@ -35,8 +35,7 @@ int main(int argc, char *argv[]) {
 
   string input_file(argv[1]);
 
-  Compiler *c{new Compiler()};
-  c->compile(input_file);
-  delete c;
+  Compiler c;
+  c.compile(input_file);
   return 0;
 }

include/serene/compiler.hpp

@@ -27,6 +27,7 @@
 
 #include "serene/llvm/IR/Value.h"
 #include "serene/logger.hpp"
+#include "serene/state.hpp"
 #include <llvm/IR/IRBuilder.h>
 #include <llvm/IR/LLVMContext.h>
 #include <string>
@@ -38,18 +39,16 @@
 #endif
 
 namespace serene {
-// Forward declaration of State. The actual declaration is in state.hpp
-class State;
 
 class Compiler {
 
 public:
   llvm::LLVMContext context;
-  llvm::IRBuilder<> *builder;
+  llvm::IRBuilder<> builder;
 
   Compiler();
 
-  State *state;
+  State state;
   llvm::Value *log_error(const char *s);
   void compile(std::string &input);
 

include/serene/namespace.hpp

@@ -25,7 +25,6 @@
 #ifndef NAMESPACE_H
 #define NAMESPACE_H
 
-#include "serene/compiler.hpp"
 #include "serene/llvm/IR/Value.h"
 #include "serene/logger.hpp"
 #include <llvm/IR/Module.h>
@@ -47,8 +46,8 @@ public:
   std::string name;
 
   Namespace(std::string &n) : name(n){};
-  llvm::Value *lookup(std::string &name);
-  void insert_symbol(std::string &name, llvm::Value *v);
+  llvm::Value *lookup(const std::string &name);
+  void insert_symbol(const std::string &name, llvm::Value *v);
 
   void print_scope();
   ~Namespace();

include/serene/reader.hpp

@@ -60,15 +60,15 @@ class Reader {
 private:
   std::stringstream input_stream;
 
-  char get_char(bool);
+  char get_char(bool skip_whitespace);
   void unget_char();
-  bool is_valid_for_identifier(char);
+  bool is_valid_for_identifier(char c);
 
   // The property to store the ast tree
   ast_tree ast;
 
   ast_node read_symbol();
-  ast_list_node read_list(List *);
+  ast_list_node read_list(List *list);
   ast_node read_expr();
 
 public:

include/serene/special_forms/def.hpp

@@ -27,10 +27,10 @@
 
 #include "serene/compiler.hpp"
 #include "serene/expr.hpp"
-#include "serene/list.hpp"
 #include "serene/llvm/IR/Value.h"
 #include "serene/logger.hpp"
 #include "serene/state.hpp"
+#include "serene/symbol.hpp"
 #include <string>
 
 #if defined(ENABLE_LOG) || defined(ENABLE_DEF_LOG)
@@ -44,13 +44,13 @@ namespace special_forms {
 
 class Def : public AExpr {
 private:
-  AExpr *sym;
-  AExpr *value;
+  Symbol *m_sym;
+  AExpr *m_value;
 
 public:
-  ExprId id() const override { return def; };
+  ExprId id() const override { return def; }
 
-  Def(AExpr *s, AExpr *v);
+  Def(serene::Symbol *symbol_, AExpr *value_);
   std::string string_repr() const override;
   llvm::Value *codegen(Compiler &compiler, State &state) override;
   ~Def();

include/serene/state.hpp

@@ -47,7 +47,7 @@ public:
 
   void add_namespace(Namespace *ns, bool set_current, bool overwrite);
   bool set_current_ns(Namespace *ns);
-  llvm::Value *lookup_in_current_scope(std::string &name);
+  llvm::Value *lookup_in_current_scope(const std::string &name);
   void set_in_current_ns_root_scope(std::string name, llvm::Value *v);
   ~State();
 };

src/compiler.cpp

@@ -37,14 +37,11 @@ using namespace llvm;
 
 namespace serene {
 
-Compiler::Compiler() {
+Compiler::Compiler() : builder(context) {
   string default_ns_name("user");
   Namespace *default_ns = new Namespace(default_ns_name);
 
-  builder = new IRBuilder(this->context);
-  state = new State();
-
-  state->add_namespace(default_ns, true, true);
+  state.add_namespace(default_ns, true, true);
 };
 
 Value *Compiler::log_error(const char *s) {
@@ -58,7 +55,7 @@ void Compiler::compile(string &input) {
 
   COMPILER_LOG("Parsing the input has been done.")
   for (const ast_node &x : ast) {
-    auto *IR{x->codegen(*this, *this->state)};
+    auto *IR{x->codegen(*this, this->state)};
 
     if (IR) {
       fmt::print("'{}' generates: \n", x->string_repr()
@@ -70,17 +67,12 @@ void Compiler::compile(string &input) {
       fmt::print("No gen\n");
     }
   }
-  state->current_ns->print_scope();
+  state.current_ns->print_scope();
   delete r;
   COMPILER_LOG("Done!")
   return;
 };
 
-Compiler::~Compiler() {
-  COMPILER_LOG("Deleting state...");
-  delete state;
-  COMPILER_LOG("Deleting builder...");
-  delete builder;
-}
+Compiler::~Compiler() { COMPILER_LOG("destroying"); }
 
 } // namespace serene

src/list.cpp

@@ -36,22 +36,26 @@ using namespace llvm;
 namespace serene {
 
 std::optional<ast_node> List::at(uint index) {
-  if (index >= nodes_.size())
+  if (index >= nodes_.size()) {
     return std::nullopt;
+  }
+
   auto itr = cbegin(nodes_);
   std::advance(itr, index);
   return std::make_optional(*itr);
 }
 
-void List::cons(ast_node f) { nodes_.push_back(std::move(f)); }
+void List::cons(ast_node node) { nodes_.push_front(std::move(node)); }
 
-void List::append(ast_node t) { nodes_.push_front(std::move(t)); }
+void List::append(ast_node node) { nodes_.push_back(std::move(node)); }
 
 std::string List::string_repr() const {
   std::string s;
 
-  for (auto &n : nodes_)
-    s += n->string_repr();
+  for (auto &n : nodes_) {
+    // TODO: Fix the tailing space for the last element
+    s = s + n->string_repr() + " ";
+  }
 
   return fmt::format("({})", s);
 }
@@ -68,16 +72,23 @@ Value *List::codegen(Compiler &compiler, State &state) {
   auto name_ptr = at(1).value_or(nullptr);
   auto body_ptr = at(2).value_or(nullptr);
 
-  if (!def_ptr && def_ptr->id() != symbol && static_cast<Symbol*>(def_ptr.get())->name() != "def")
-    return nullptr;
+  if (def_ptr && def_ptr->id() == symbol &&
+      static_cast<Symbol *>(def_ptr.get())->name() == "def") {
 
-  if (!name_ptr && def_ptr->id() != symbol)
-    return nullptr;
+    if (!name_ptr && def_ptr->id() != symbol) {
+      return compiler.log_error("First argument of 'def' has to be a symbol.");
+    }
 
-  if (!body_ptr)
-    return nullptr;
+    if (!body_ptr) {
+      return compiler.log_error("'def' needs 3 arguments, two has been given.");
+    }
 
-  special_forms::Def def(name_ptr.get(), body_ptr.get());
-  return def.codegen(compiler, state);
+    special_forms::Def def(static_cast<Symbol *>(name_ptr.get()),
+                           body_ptr.get());
+    return def.codegen(compiler, state);
+  }
+
+  EXPR_LOG("Not implemented in list.");
+  return nullptr;
 }
 } // namespace serene

src/namespace.cpp

@@ -32,8 +32,10 @@ using namespace llvm;
 
 namespace serene {
 
-Value *Namespace::lookup(string &name) { return scope[name]; };
-void Namespace::insert_symbol(string &name, Value *v) { scope[name] = v; }
+Value *Namespace::lookup(const string &name) { return scope[name]; };
+
+void Namespace::insert_symbol(const string &name, Value *v) { scope[name] = v; }
+
 void Namespace::print_scope() {
   typedef map<string, Value *>::const_iterator Iter;
 

src/special_forms/def.cpp

@@ -38,27 +38,21 @@ using namespace llvm;
 namespace serene {
 namespace special_forms {
 
-
-Def::Def(AExpr *s, AExpr *v)
-  : sym(s), value(v) {
-}
+Def::Def(Symbol *symbol_, AExpr *value_) : m_sym(symbol_), m_value(value_) {}
 
 string Def::string_repr() const {
   // this method is not going to get called.
   return "Def";
-};
+}
 
 Value *Def::codegen(Compiler &compiler, State &state) {
+  state.set_in_current_ns_root_scope(m_sym->name(),
+                                     m_value->codegen(compiler, state));
 
-  auto symobj = dynamic_cast<Symbol *>(this->sym);
-
-  if (symobj->id() != symbol) {
-    return compiler.log_error("First argument of 'def' should be a symbol.");
-  }
-
-  state.set_in_current_ns_root_scope(symobj->name(), value->codegen(compiler, state));
-  return symobj->codegen(compiler, state);
-};
+  // TODO: Do we need to return the codegen of the symbol instead
+  // of the symbol itself?
+  return m_sym->codegen(compiler, state);
+}
 
 Def::~Def() { EXPR_LOG("Destroying def"); };
 } // namespace special_forms

src/state.cpp

@@ -67,7 +67,7 @@ bool State::set_current_ns(Namespace *ns) {
   return false;
 };
 
-Value *State::lookup_in_current_scope(string &name) {
+Value *State::lookup_in_current_scope(const string &name) {
   if (current_ns) {
     return current_ns->lookup(name);
   }

src/symbol.cpp

@@ -46,21 +46,19 @@ const string &Symbol::name() const { return name_; }
 Symbol::Symbol(const string &name) : name_(name) {}
 
 Value *Symbol::codegen(Compiler &compiler, State &state) {
-  auto name = this->name_;
-
-  if (name == "false") {
+  if (name() == "false") {
     return ConstantInt::get(Type::getInt1Ty(compiler.context), 0);
   }
 
-  if (name == "true") {
+  if (name() == "true") {
     return ConstantInt::get(Type::getInt1Ty(compiler.context), 1);
   }
 
-  Value *V = state.lookup_in_current_scope(name);
+  Value *V = state.lookup_in_current_scope(name());
 
   if (!V) {
     return compiler.log_error(
-        fmt::format("Unable to resolve symbol '{}'.", name).c_str());
+        fmt::format("Unable to resolve symbol '{}'.", name()).c_str());
   }
   return V;
 }