Add some codegen code for the if else statements

[bloody76]

This change is 

[coveralls]

Coverage decreased (-0.2%) to 98.612% when pulling a091e9ae8654e6a12e850a5364ae0d20988a1f95 on add_codegen_for_if_stmts into d33b9ba507c4bf0ef08a73f7d53b62563dd12557 on master.

[bloody76]

Reviewed 27 of 27 files at r1, 4 of 4 files at r2. Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.004%) to 98.844% when pulling 79dc09c7675926cf50aae6c070f1128ed25e8245 on add_codegen_for_if_stmts into d33b9ba507c4bf0ef08a73f7d53b62563dd12557 on master.

[coveralls]

Coverage increased (+0.004%) to 98.844% when pulling 099ad1bd372cfecd108a53fa2e1ff9f71e682cb5 on add_codegen_for_if_stmts into d33b9ba507c4bf0ef08a73f7d53b62563dd12557 on master.

[nitnelave]

Review status: 27 of 28 files reviewed at latest revision, 9 unresolved discussions.

---

src/ast/if_statement.h, line 23 at r2 (raw file):

        bodies_(std::move(bodies)),
        else_statement_(std::move(else_statement)) {
    assert(conditions.size() == bodies.size());

vector<pair<condition, body>>?

---

src/codegen/codegen.cc, line 42 at r2 (raw file):

    : module_(std::make_unique<Module>(name, context_)),
      ir_builder_(context_, ConstantFolder()),
      current_block_(nullptr) {

No, I don't want any nullptr. If you want a nullable pointer, make it an Option<pointer> and set it to none

---

src/codegen/codegen.h, line 58 at r2 (raw file):

  // Return value of visitation of a value node.
  llvm::Value* gen_value_;
  llvm::BasicBlock* current_block_;

Documentation

---

src/codegen/codegen.h, line 62 at r2 (raw file):

  std::string current_function_name_;

  bool return_unhandled_ = false;

return_handled, it's better to have positive names/concepts.

---

src/codegen/codegen.h, line 65 at r2 (raw file):

};

#define CONSUME_UNHANDLED_RETURN(VAR) \

I'm not a fan of that macro... If you really want to use a one-liner to replace a two-liner, write something like: auto was_handled = consume_unhandled_return(&return_unhandled_);

---

src/codegen/codegen_statement.cc, line 27 at r2 (raw file):

    if (return_unhandled_) {
      return;

Add a warning to the error list if it's not the last statement.

---

src/codegen/codegen_statement.cc, line 35 at r2 (raw file):

  if (node->value().is_ok()) {
    node->value().value_or_die()->accept(*this);
    assert(gen_value_ != nullptr && "No value generated by the visitor");

turn genvalue into an Option to remove nullptr

---

src/codegen/codegen_statement.cc, line 57 at r2 (raw file):

  // We generate the first block of code for the 'if' statement.
  (*condition++)->accept(*this);

Separate the increment from the usage.

---

src/codegen/codegen_statement.cc, line 67 at r2 (raw file):

  BasicBlock* last_block = current_block_;
  BasicBlock* jump_block = current_block;
  while (condition != std::end(node->conditions())) {

I'll have a closer look at the implementation later, but I'd really like to see the binary (non-vector) one.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.06%) to 98.903% when pulling 8f7d52fd0fe71b17d16307e93e01c6b650b75eaf on add_codegen_for_if_stmts into d33b9ba507c4bf0ef08a73f7d53b62563dd12557 on master.

[coveralls]

Coverage increased (+0.06%) to 98.903% when pulling 7879ccd2686210e5141d9c331565a80af7a5d2ad on add_codegen_for_if_stmts into d33b9ba507c4bf0ef08a73f7d53b62563dd12557 on master.

[coveralls]

Coverage increased (+0.06%) to 98.905% when pulling c697420137e9a75d309b1dc592ae78cbc63775b0 on add_codegen_for_if_stmts into d33b9ba507c4bf0ef08a73f7d53b62563dd12557 on master.

[bloody76]

Reviewed 1 of 1 files at r3, 2 of 30 files at r4, 31 of 31 files at r5. Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

Comments from Reviewable

[bloody76]

Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

src/ast/if_statement.h, line 23 at r2 (raw file):

Previously, nitnelave (nitnelave) wrote…

vector>?

modified in the new recursive version

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.06%) to 98.905% when pulling db29c4e0bd22a376396c39c93e2e4d08be0f66e5 on add_codegen_for_if_stmts into d33b9ba507c4bf0ef08a73f7d53b62563dd12557 on master.

[bloody76]

Review status: 29 of 32 files reviewed at latest revision, 5 unresolved discussions.

---

src/codegen/codegen.cc, line 42 at r2 (raw file):

Previously, nitnelave (nitnelave) wrote…

No, I don't want any nullptr. If you want a nullable pointer, make it an `Option` and set it to `none`

Done.

---

src/codegen/codegen.h, line 58 at r2 (raw file):

Previously, nitnelave (nitnelave) wrote…

Documentation

Done.

---

src/codegen/codegen.h, line 62 at r2 (raw file):

Previously, nitnelave (nitnelave) wrote…

return_handled, it's better to have positive names/concepts.

Done.

---

src/codegen/codegen.h, line 65 at r2 (raw file):

Previously, nitnelave (nitnelave) wrote…

I'm not a fan of that macro... If you really want to use a one-liner to replace a two-liner, write something like: `auto was_handled = consume_unhandled_return(&return_unhandled_);`

Done.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.06%) to 98.905% when pulling c3712de70fa6dc9c641d38599e16cb17535f0815 on add_codegen_for_if_stmts into d33b9ba507c4bf0ef08a73f7d53b62563dd12557 on master.

[bloody76]

Review status: 29 of 32 files reviewed at latest revision, 6 unresolved discussions.

---

src/codegen/codegen_statement.cc, line 27 at r2 (raw file):

Previously, nitnelave (nitnelave) wrote…

Add a warning to the error list if it's not the last statement.

Done.

---

Comments from Reviewable

[bloody76]

Review status: 28 of 33 files reviewed at latest revision, 7 unresolved discussions.

---

src/codegen/codegen_statement.cc, line 35 at r2 (raw file):

Previously, nitnelave (nitnelave) wrote…

turn gen_value_ into an Option to remove nullptr

Done.

---

src/codegen/codegen_statement.cc, line 57 at r2 (raw file):

Previously, nitnelave (nitnelave) wrote…

Separate the increment from the usage.

I think this is deprecated with the recursive version

---

Comments from Reviewable

[bloody76]

Reviewed 1 of 6 files at r6. Review status: 29 of 33 files reviewed at latest revision, 6 unresolved discussions.

---

Comments from Reviewable

[bloody76]

Reviewed 3 of 6 files at r6. Review status: 32 of 33 files reviewed at latest revision, 6 unresolved discussions.

---

Comments from Reviewable

[bloody76]

Reviewed 1 of 6 files at r6. Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

Comments from Reviewable

[coveralls]

Coverage decreased (-0.8%) to 98.062% when pulling 07299a4bfdbce6dd01d75be7e495dafdffadd477 on add_codegen_for_if_stmts into d33b9ba507c4bf0ef08a73f7d53b62563dd12557 on master.

[coveralls]

Coverage decreased (-0.8%) to 98.064% when pulling 07299a4bfdbce6dd01d75be7e495dafdffadd477 on add_codegen_for_if_stmts into d33b9ba507c4bf0ef08a73f7d53b62563dd12557 on master.

[nitnelave]

Check the coverage, to see which part you didn't test sufficiently.

---

Reviewed 5 of 31 files at r5. Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

src/codegen/codegen.h, line 86 at r6 (raw file):

  bool has_returned_ = false;

  std::list<CodeGenWarning> warning_messages_;

Use an ErrorList from src/visitor/error_visitor.h

---

src/codegen/codegen_statement.cc, line 25 at r6 (raw file):

    if (already_returned) {
      warning_messages_.push_back(
          CodeGenWarning("The statement won't be executed because the function "

Maybe a message more like "Unreachable code"? And also break after the first warning, or you'll print it for each statement.

---

src/codegen/codegen_statement.cc, line 84 at r6 (raw file):

  has_returned_ = if_or_elseif_returned || else_only_returned;

  if (!has_returned_) {

This part needs a bit more explanation on when you create the end block or not.

---

src/error/error.h, line 169 at r6 (raw file):

  MaybeError(  // NOLINT: explicit
      E&& value, typename std::enable_if<
                     std::is_constructible<Err, E&&>::value>::type* /*unused*/

can you get rid of the whitespace changes?

---

src/parser/parser.cc, line 279 at r6 (raw file):

      RETURN_OR_MOVE(auto else_body, parse_statement_or_list());
      else_statement = std::make_unique<ast::IfStatement>(
          location.range(), none, std::move(else_body), none);

Why the location change?

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.08%) to 98.916% when pulling 1d191e6862b2e6a125ec90e244603237c6f0adc7 on add_codegen_for_if_stmts into d33b9ba507c4bf0ef08a73f7d53b62563dd12557 on master.

[bloody76]

Reviewed 3 of 3 files at r7. Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

src/codegen/codegen.h, line 86 at r6 (raw file):

Previously, nitnelave (nitnelave) wrote…

Use an `ErrorList` from src/visitor/error_visitor.h

Done.

---

Comments from Reviewable

[bloody76]

Review status: 30 of 35 files reviewed at latest revision, 5 unresolved discussions.

---

src/codegen/codegen_statement.cc, line 25 at r6 (raw file):

Previously, nitnelave (nitnelave) wrote…

Maybe a message more like "Unreachable code"? And also break after the first warning, or you'll print it for each statement.

Done.

---

Comments from Reviewable

[bloody76]

Review status: 30 of 35 files reviewed at latest revision, 5 unresolved discussions.

---

src/error/error.h, line 169 at r6 (raw file):

Previously, nitnelave (nitnelave) wrote…

can you get rid of the whitespace changes?

Done.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.07%) to 98.91% when pulling 6671176375a32d13f333afac696095577db49a0a on add_codegen_for_if_stmts into d33b9ba507c4bf0ef08a73f7d53b62563dd12557 on master.

[bloody76]

Review status: 28 of 34 files reviewed at latest revision, 5 unresolved discussions.

---

src/parser/parser.cc, line 279 at r6 (raw file):

Previously, nitnelave (nitnelave) wrote…

Why the location change?

No reason, I just moved it by accident during the refacto

---

Comments from Reviewable

[bloody76]

Review status: 28 of 34 files reviewed at latest revision, 5 unresolved discussions.

---

src/codegen/codegen_statement.cc, line 84 at r6 (raw file):

Previously, nitnelave (nitnelave) wrote…

This part needs a bit more explanation on when you create the end block or not.

Done.

---

Comments from Reviewable

[bloody76]

Reviewed 6 of 7 files at r8. Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.07%) to 98.91% when pulling 41bf23af2b83ff51ccc54dc215066bc0fc020ad2 on add_codegen_for_if_stmts into d33b9ba507c4bf0ef08a73f7d53b62563dd12557 on master.

[bloody76]

Reviewed 1 of 7 files at r8, 1 of 1 files at r9. Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.07%) to 98.91% when pulling 2efd9dd2fdc336ad1737df0fa90b9fa9f09e6f1d on add_codegen_for_if_stmts into d33b9ba507c4bf0ef08a73f7d53b62563dd12557 on master.

[coveralls]

Coverage increased (+0.07%) to 98.91% when pulling 2efd9dd2fdc336ad1737df0fa90b9fa9f09e6f1d on add_codegen_for_if_stmts into d33b9ba507c4bf0ef08a73f7d53b62563dd12557 on master.

[nitnelave]

Reviewed 2 of 30 files at r4, 21 of 31 files at r5, 2 of 6 files at r6, 1 of 3 files at r7, 5 of 7 files at r8, 1 of 1 files at r9. Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

src/parser/parser.cc, line 3 at r9 (raw file):

#include "parser/parser.h"

#include <iostream>

Remove this header.

---

test/resources/resources.cc, line 270 at r9 (raw file):

  result.value_or_die()->accept(generator);

  if (!generator.error_list().warnings().empty()) {

Ideally we would differentiate errors and warnings... it's okay for now, but :/

---

Comments from Reviewable

[bloody76]

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

test/resources/resources.cc, line 270 at r9 (raw file):

Previously, nitnelave (nitnelave) wrote…

Ideally we would differentiate errors and warnings... it's okay for now, but :/

We can change the name form error_list to msg_list if you want. Holding both errors and warnings in the same structure is not really an issue

---

Comments from Reviewable

[nitnelave]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

test/resources/resources.cc, line 270 at r9 (raw file):

Previously, bloody76 (Bd76) wrote…

We can change the name form error_list to msg_list if you want. Holding both errors and warnings in the same structure is not really an issue

No, but in the test, it's tested as ERROR, where it should be tested as WARNING

---

Comments from Reviewable

[nitnelave]

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

test/resources/ir/function_with_consecutive_returns.gh, line 3 at r9 (raw file):

fun main() {
  return 1;
  return 2;

Add another test with another statement after that, I want to check that the message doesn't appear twice.

---

Comments from Reviewable

[bloody76]

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

test/resources/resources.cc, line 270 at r9 (raw file):

Previously, nitnelave (nitnelave) wrote…

No, but in the test, it's tested as `ERROR`, where it should be tested as `WARNING`

Han ok, yes indeed

---

Comments from Reviewable

[bloody76]

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

src/parser/parser.cc, line 3 at r9 (raw file):

Previously, nitnelave (nitnelave) wrote…

Remove this header.

Done.

---

test/resources/ir/function_with_consecutive_returns.gh, line 3 at r9 (raw file):

Previously, nitnelave (nitnelave) wrote…

Add another test with another statement after that, I want to check that the message doesn't appear twice.

Done.

---

Comments from Reviewable

[bloody76]

Reviewed 1 of 2 files at r10. Review status: 34 of 35 files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[bloody76]

Reviewed 1 of 2 files at r10. Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.07%) to 98.911% when pulling b6c531b8ec81c1fc0ab6ad0fefa3bfbe88429920 on add_codegen_for_if_stmts into d33b9ba507c4bf0ef08a73f7d53b62563dd12557 on master.

[nitnelave]

---

Reviewed 1 of 7 files at r8, 1 of 2 files at r10. Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable