Parse blocks and statements

[bloody76]

This is a draft, mostly a proposition to change the way we parse statements.

---

This change is 

[nitnelave]

Check your clang version for clang-format. The CI uses 3.8, I think 3.9 is also acceptable. Otherwise, we get spurious whitespace changes.

Overall, I like the PR. It's still a draft, and it's a bit big (could have split it into one with ValueStatement and one with BlockStatement), but it's good.

Of course, you'll need tests before you can submit.

---

Reviewed 1 of 9 files at r2. Review status: 1 of 17 files reviewed at latest revision, 42 unresolved discussions, some commit checks failed.

---

src/ast/ast.h, line 54 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

organize better

ValuedStatement is not abstract, move to previous section.

---

src/ast/ast.h, line 70 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

organize better

Sort by name.

---

src/ast/block.h, line 9 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

I consider this a statement, but is it really ?

Yes. For the name: BlockStatement is better, I think.

---

src/ast/block.h, line 11 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

maybe add some typedef for the vector of statements

using StatementList = std::vector<std::unique_ptr>?

---

src/ast/block.h, line 16 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

connnnst

I'm not super satisfied with the state of the constness so far. All the visitors take a non-const AST, so this method needs to at least exist in the non-const version.

---

src/ast/function_declaration.h, line 21 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

naming maybe outdated

I would put unique_ptr

---

src/ast/function_declaration.h, line 51 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

accept body directly ?

What do you mean?

---

src/ast/valued_statement.h, line 7 at r2 (raw file):

namespace ast {

class ValuedStatement : public Statement {

What about ValueStatement ? Otherwise it seems that the statement has a value, instead of containing one.

---

src/ast/visitor.cc, line 39 at r2 (raw file):

void ASTVisitor::visit(VariableReference* /*unused*/) {}

void ASTVisitor::visit(Block* /* unused */) {}

Please implement.

---

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

}

void CodeGenerator::visit(ast::Block* node) {

This should be in ast/visitor.cc

---

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

void CodeGenerator::visit(ast::Block* node) {
    for (auto const& statement : node->statements()) {

const auto&

---

src/error/error.h, line 91 at r2 (raw file):

  /// Construct a value directly.
  template <typename T,
            typename = typename std::enable_if<

Check your version of clang. 3.8? 3.9? I'd like to avoid spurious changes like that.

---

src/parser/parser.cc, line 244 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

case not handled, should be.

Should we? Anyway, there are many cases not handled in the grammar, don't try to handle them all right now. Instead, add one case to handle, make that a PR, then add another one in another PR.

---

src/parser/parser.cc, line 332 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

Could be something else, what about fun main() = if (condition) value else value ?

Still needs a ; at the end, for the grammar to be unambiguous (not the current grammar, but the planned grammar). Change it in doc/grammar.py if you want to see

---

src/parser/parser.cc, line 343 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

message might not be the best

What was wrong with the previous implementation? In a recursive descent parser, you only call a function once you know the required conditions are present (i.e. OPEN_BRACE in this case), that way the function doesn't have to check for it, and we can detect the error at a higher level. What message is better: "Expected function body" or "Statement list expected, got `fun' instead"?

---

src/parser/parser.cc, line 31 at r2 (raw file):

ScopedLocation Parser::scoped_location() const { return ScopedLocation(this); }

/*

Documentation comments are written with ///, normal comments with //. Comments at the header level are preferred.

---

src/parser/parser.cc, line 56 at r2 (raw file):

/*
 * <text>[::<text>][<VALUE>::...]

(::)?[<Upper>::](<Upper>|<lower>)

---

src/parser/parser.cc, line 155 at r2 (raw file):

  // Parse function calls.
  if (current_token().type() == TokenType::OPEN_PAREN) {

No. create_functor()()

---

src/parser/parser.cc, line 208 at r2 (raw file):

  // Then a semicolon.
  EXPECT_TOKEN(TokenType::SEMICOLON,
               "Expected semicolon at the end of the statement");

Why?

---

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

 * { <StatementList> ... }
 */
Parser::ErrorOrPtr<ast::Statement>

BlockStatement

---

src/parser/parser.cc, line 283 at r2 (raw file):

  auto location = scoped_location();

  if (current_token().type() == TokenType::OPEN_BRACE) {

This function is called only when an open brace has been detected. At best, this should be an assert

---

src/parser/parser.cc, line 284 at r2 (raw file):

  if (current_token().type() == TokenType::OPEN_BRACE) {
      std::vector<std::unique_ptr<ast::Statement>> statements;

Declare closer to use (under the get_token)

---

src/parser/parser.cc, line 288 at r2 (raw file):

      while (current_token().type() != TokenType::CLOSE_BRACE) {
          RETURN_OR_MOVE(auto sub_statement, parse_statement_list());

Why parse a list?

---

src/transform/function_value_body.cc, line 17 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

can't we give back directly a return statement ? Would the code gen handle it correctly ?

The idea is to limit the AST to as few constructs as possible. Why have to handle StatementBlock or ReturnStatement when you can do perfectly fine with StatementBlock?

---

Comments from Reviewable

[nitnelave]

Oh, and also, we have tests, and you broke around half of them. make check is your friend!

---

Review status: 1 of 17 files reviewed at latest revision, 42 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[bloody76]

src/parser/parser.cc, line 244 at r1 (raw file):

Previously, nitnelave (nitnelave) wrote…

Should we? Anyway, there are many cases not handled in the grammar, don't try to handle them all right now. Instead, add one case to handle, make that a PR, then add another one in another PR.

True. I don't know, I think we should for about syntaxes like

int i = 0;
for (; i < x; ++i)
    ;

This is super error prone but I wonder if there is any bestpractice that exists using this kind of trick.

---

Comments from Reviewable

[bloody76]

src/parser/parser.cc, line 258 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

remove

Done.

---

Comments from Reviewable

[bloody76]

src/parser/parser.cc, line 250 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

remove

Done.

---

Comments from Reviewable

[bloody76]

src/parser/parser.cc, line 155 at r2 (raw file):

Previously, nitnelave (nitnelave) wrote…

No. `create_functor()()`

I don't get it, what would be that notation ? So we could have function declarations like

fun test(a: Int)(b: String) {} ?

---

Comments from Reviewable

[bloody76]

src/parser/parser.cc, line 332 at r1 (raw file):

Previously, nitnelave (nitnelave) wrote…

Still needs a `;` at the end, for the grammar to be unambiguous (not the current grammar, but the planned grammar). Change it in doc/grammar.py if you want to see

Ok will do

---

Comments from Reviewable

[bloody76]

Review status: 1 of 17 files reviewed at latest revision, 41 unresolved discussions, some commit checks failed.

---

src/parser/parser.cc, line 208 at r2 (raw file):

Previously, nitnelave (nitnelave) wrote…

Why?

I think we can discuss this, but the semicolon should be used to declare the end of statement, and should therefor be parsed in the parse_statement function. Making it the responsability of the VariableDecl could prevent the user to do things like:

if ((val var: Int = init_value()) == 0) {}

---

Comments from Reviewable

[bloody76]

src/transform/function_value_body.cc, line 17 at r1 (raw file):

Previously, nitnelave (nitnelave) wrote…

The idea is to limit the AST to as few constructs as possible. Why have to handle StatementBlock or ReturnStatement when you can do perfectly fine with StatementBlock?

Ha yes right, anyway we transform a Statement into a StatementList.

---

Comments from Reviewable

[nitnelave]

Review status: 1 of 17 files reviewed at latest revision, 41 unresolved discussions, some commit checks failed.

---

src/parser/parser.cc, line 244 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

True. I don't know, I think we should for about syntaxes like ``` int i = 0; for (; i < x; ++i) ; ``` This is super error prone but I wonder if there is any bestpractice that exists using this kind of trick.

Note: the for-loop is only on ranges (foreach). the semicolon alone can be replaced by continue; A good reason to have the single ; in C++ is to play nice with macros that declare a list of statements ending with ;, you can still add a ; at the end to make the macro call look like a statement.

---

src/parser/parser.cc, line 155 at r2 (raw file):

Previously, bloody76 (Bd76) wrote…

I don't get it, what would be that notation ? So we could have function declarations like `fun test(a: Int)(b: String) {}` ?

No, it's just that a function call doesn't need to be just with a function. It can be any expression that evaluates to something with a function-call operator. That also allows easy handling of object methods, since you only need to resolve the field, and then whether it's a method or not will be determined when resolving the operator.

---

src/parser/parser.cc, line 208 at r2 (raw file):

Previously, bloody76 (Bd76) wrote…

I think we can discuss this, but the semicolon should be used to declare the end of statement, and should therefor be parsed in the parse_statement function. Making it the responsability of the VariableDecl could prevent the user to do things like: `if ((val var: Int = init_value()) == 0) {}`

A variable declaration has no value.

---

Comments from Reviewable

[bloody76]

src/parser/parser.cc, line 155 at r2 (raw file):

Previously, nitnelave (nitnelave) wrote…

No, it's just that a function call doesn't need to be just with a function. It can be any expression that evaluates to something with a function-call operator. That also allows easy handling of object methods, since you only need to resolve the field, and then whether it's a method or not will be determined when resolving the operator.

Ha yes indeed

---

Comments from Reviewable

[bloody76]

Will do for the tests ! Still not used to work on the project, I will check for the coverage too.

For Clang, I use llvm 4.0.1, last version from brew. Should I downgrade ?

---

Comments from Reviewable

[bloody76]

src/ast/ast.h, line 54 at r1 (raw file):

Previously, nitnelave (nitnelave) wrote…

ValuedStatement is not abstract, move to previous section.

Done.

---

Comments from Reviewable

[bloody76]

src/ast/block.h, line 11 at r1 (raw file):

Previously, nitnelave (nitnelave) wrote…

using StatementList = std::vector>?

I named it StatementCollection, do you want to be more specific by saying it's a list ?

---

Comments from Reviewable

[bloody76]

Review status: 1 of 17 files reviewed at latest revision, 41 unresolved discussions, some commit checks failed.

---

src/ast/ast.h, line 70 at r1 (raw file):

Previously, nitnelave (nitnelave) wrote…

Sort by name.

Done.

---

src/ast/block.h, line 4 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

no need for value.h

Done.

---

src/ast/block.h, line 9 at r1 (raw file):

Previously, nitnelave (nitnelave) wrote…

Yes. For the name: BlockStatement is better, I think.

Done.

---

src/ast/block.h, line 16 at r1 (raw file):

Previously, nitnelave (nitnelave) wrote…

I'm not super satisfied with the state of the constness so far. All the visitors take a non-const AST, so this method needs to at least exist in the non-const version.

Ok, will do too

---

src/ast/CMakeLists.txt, line 19 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

missing valued_statement.h

Done.

---

src/ast/function_declaration.h, line 11 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

remove

needed in fact

---

src/ast/function_declaration.h, line 51 at r1 (raw file):

Previously, nitnelave (nitnelave) wrote…

What do you mean?

I mean instead of doing of body_.get_unchecked<Smth>, maybe directly put body_.get_unchecked<ASTNode>()->accept(visitor);

---

src/ast/value.h, line 4 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

remove

Done.

---

src/ast/valued_statement.h, line 7 at r2 (raw file):

Previously, nitnelave (nitnelave) wrote…

What about `ValueStatement` ? Otherwise it seems that the statement has a value, instead of containing one.

indeed !

---

src/ast/visitor.cc, line 8 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

sort/organize

Done.

---

src/ast/visitor.cc, line 39 at r2 (raw file):

Previously, nitnelave (nitnelave) wrote…

Please implement.

Done.

---

src/codegen/codegen.h, line 33 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

organize

Done.

---

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

Previously, nitnelave (nitnelave) wrote…

This should be in ast/visitor.cc

Done.

---

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

Previously, nitnelave (nitnelave) wrote…

const auto&

Done.

---

src/error/error.h, line 91 at r2 (raw file):

Previously, nitnelave (nitnelave) wrote…

Check your version of clang. 3.8? 3.9? I'd like to avoid spurious changes like that.

I have the version 4.0.1, should I downgrade ?

---

src/parser/parser.cc, line 16 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

remove

mistake I think

---

src/parser/parser.cc, line 244 at r1 (raw file):

Previously, nitnelave (nitnelave) wrote…

Note: the for-loop is only on ranges (foreach). the semicolon alone can be replaced by `continue;` A good reason to have the single `;` in C++ is to play nice with macros that declare a list of statements ending with `;`, you can still add a `;` at the end to make the macro call look like a statement.

Done.

---

src/parser/parser.cc, line 260 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

put as before

Done.

---

src/parser/parser.cc, line 292 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

new line useless

Done.

---

src/parser/parser.cc, line 332 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

Ok will do

Done.

---

src/parser/parser.cc, line 343 at r1 (raw file):

Previously, nitnelave (nitnelave) wrote…

What was wrong with the previous implementation? In a recursive descent parser, you only call a function once you know the required conditions are present (i.e. OPEN_BRACE in this case), that way the function doesn't have to check for it, and we can detect the error at a higher level. What message is better: "Expected function body" or "Statement list expected, got `fun' instead"?

Done.

---

src/parser/parser.cc, line 31 at r2 (raw file):

Previously, nitnelave (nitnelave) wrote…

Documentation comments are written with `///`, normal comments with `//`. Comments at the header level are preferred.

We are not going to put them at the header level ? They are to remind the grammar, maybe in the body then ?

---

src/parser/parser.cc, line 155 at r2 (raw file):

Previously, bloody76 (Bd76) wrote…

Ha yes indeed

Done.

---

src/parser/parser.cc, line 208 at r2 (raw file):

Previously, nitnelave (nitnelave) wrote…

A variable declaration has no value.

Done.

---

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

Previously, nitnelave (nitnelave) wrote…

BlockStatement

Done.

---

src/parser/parser.cc, line 283 at r2 (raw file):

Previously, nitnelave (nitnelave) wrote…

This function is called only when an open brace has been detected. At best, this should be an assert

Done.

---

src/parser/parser.cc, line 284 at r2 (raw file):

Previously, nitnelave (nitnelave) wrote…

Declare closer to use (under the get_token)

Done.

---

src/parser/parser.cc, line 288 at r2 (raw file):

Previously, nitnelave (nitnelave) wrote…

Why parse a list?

Done.

---

src/pretty_printer/pretty_printer.h, line 15 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

organize

Done.

---

src/pretty_printer/pretty_printer.h, line 69 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

merge with ;

Done.

---

src/pretty_printer/pretty_printer.h, line 80 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

infix operators

Done.t

---

src/transform/function_value_body.cc, line 6 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

no need for block here ?

organized instead

---

src/transform/function_value_body.cc, line 17 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

Ha yes right, anyway we transform a Statement into a StatementList.

Uh sorry I think I didn't understand it, we want to transform something like:

fun test() = 1; into fun test() { return 1; } right ?

Here I just make a BlockStatement with a unique statement which is ReturnStatement. Because a function body should be a BlockStatement only right ? And not just a Statement ?

---

Comments from Reviewable

[bloody76]

src/ast/function_declaration.h, line 21 at r1 (raw file):

Previously, nitnelave (nitnelave) wrote…

I would put unique_ptr

I don't get it, its already a unique_ptr ?

---

Comments from Reviewable

[bloody76]

Review status: 0 of 18 files reviewed at latest revision, 41 unresolved discussions.

---

src/parser/parser.cc, line 56 at r2 (raw file):

Previously, nitnelave (nitnelave) wrote…

`(::)?[::](|)`

Done.

---

Comments from Reviewable

[bloody76]

Review status: 0 of 18 files reviewed at latest revision, 43 unresolved discussions.

---

tools/clang-format.sh, line 10 at r3 (raw file):

CURRENT_DIR=$(dirname "$0")
PROJECT_DIR=${CURRENT_DIR}/..

I need to put back this readlink, but I think on mac it's not working

---

tools/clang-format.sh, line 14 at r3 (raw file):

FORMAT_CMD="clang-format -style=Google"

FIND_CMD="find ${PROJECT_DIR}/src/ ${PROJECT_DIR}/test/ -name '*.cc' -or -name '*.h'  -type f"

same here, was to lazy to search for the regex solution ...

---

Comments from Reviewable

[nitnelave]

At least for formatting, please install 3.8 or 3.9, and make clang-format resolve to clang-format-3.8/9

For the grammar comments, I would start with a comment introducing the notation (optional, repeated, alternatives, literals) in the header, then comment each function in the header.

---

Review status: 0 of 20 files reviewed at latest revision, 43 unresolved discussions, some commit checks failed.

---

src/ast/block_statement.h, line 11 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

I named it StatementCollection, do you want to be more specific by saying it's a list ?

It's ordered, so list is better

---

src/ast/function_declaration.h, line 21 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

I don't get it, its already a unique_ptr ?

must have misread, disregard

---

src/ast/function_declaration.h, line 51 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

I mean instead of doing of `body_.get_unchecked, maybe directly put body_.get_unchecked()->accept(visitor);`

You can't, with variants, you have to specify the exact type. The other way to do it would be to provide a visitor, but I think that's a bit overkill.

---

src/parser/parser.cc, line 302 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

new line useless ?

Don't scratch your head too much about newlines or not, just try to make the code clear.

---

src/parser/parser.cc, line 31 at r2 (raw file):

Previously, bloody76 (Bd76) wrote…

We are not going to put them at the header level ? They are to remind the grammar, maybe in the body then ?

I would put the grammar in documentation at the header level. Implementation level comments usually go inside the function, to document a specific implementation detail.

---

src/transform/function_value_body.cc, line 17 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

Uh sorry I think I didn't understand it, we want to transform something like: `fun test() = 1;` into `fun test() { return 1; }` right ? Here I just make a BlockStatement with a unique statement which is ReturnStatement. Because a function body should be a BlockStatement only right ? And not just a Statement ?

Yes, that's perfect

---

tools/clang-format.sh, line 10 at r3 (raw file):

Previously, bloody76 (Bd76) wrote…

I need to put back this readlink, but I think on mac it's not working

maybe check if readlink exists, otherwise define a shell function readlink?

---

tools/clang-format.sh, line 14 at r3 (raw file):

Previously, bloody76 (Bd76) wrote…

same here, was to lazy to search for the regex solution ...

.*\.(cc|h)

---

Comments from Reviewable

[nitnelave]

Review status: 0 of 20 files reviewed at latest revision, 42 unresolved discussions, some commit checks failed.

---

src/parser/parser.cc, line 284 at r2 (raw file):

Previously, bloody76 (Bd76) wrote…

Done.

Hmm, I'm not a big fan of that EXPECT_TOKEN. Usually, it's used to mean "we expect the user to put that token there", whereas here it means "we expect the function who called us to have it there". It should rather be an assert or something similar, something that should never happen. With my new PR, it could be a check(current_token().type() == TokenType::OPEN_BRACE);

---

Comments from Reviewable

[bloody76]

Review status: 0 of 20 files reviewed at latest revision, 42 unresolved discussions, some commit checks failed.

---

src/parser/parser.cc, line 284 at r2 (raw file):

Previously, nitnelave (nitnelave) wrote…

Hmm, I'm not a big fan of that EXPECT_TOKEN. Usually, it's used to mean "we expect the user to put that token there", whereas here it means "we expect the function who called us to have it there". It should rather be an assert or something similar, something that should never happen. With my new PR, it could be a `check(current_token().type() == TokenType::OPEN_BRACE);`

You mean the error message should be on a more higher level ? So that we can make more relevant messages ?

---

Comments from Reviewable

[bloody76]

Ok I will pass to clang 3.8, I was on the 4.0.1 version

---

Review status: 0 of 20 files reviewed at latest revision, 42 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[nitnelave]

Review status: 0 of 20 files reviewed at latest revision, 42 unresolved discussions, some commit checks failed.

---

src/parser/parser.cc, line 284 at r2 (raw file):

Previously, bloody76 (Bd76) wrote…

You mean the error message should be on a more higher level ? So that we can make more relevant messages ?

No, I mean that EXPECT_TOKEN is a user error, whereas the current token not being { is a developer error.

---

Comments from Reviewable

[coveralls]

Coverage decreased (-2.03%) to 96.421% when pulling 340f5965732c5bb3655348d0733260bc452377d0 on draft_statements into 73265c0a09b65b977f7d451397bcdf76cb55d5d3 on master.

[coveralls]

Coverage decreased (-2.03%) to 96.421% when pulling ce2563879f73d52de8c983353f144e2d0a5c3c57 on draft_statements into 73265c0a09b65b977f7d451397bcdf76cb55d5d3 on master.

[coveralls]

Coverage decreased (-2.03%) to 96.417% when pulling 9be841901c559c559534c80d019a6e75783b2f28 on draft_statements into 73265c0a09b65b977f7d451397bcdf76cb55d5d3 on master.

[coveralls]

Coverage decreased (-2.03%) to 96.417% when pulling 2f2b1bf58eb4099fe40830d26c51be6289e0d660 on draft_statements into 73265c0a09b65b977f7d451397bcdf76cb55d5d3 on master.

[coveralls]

Coverage decreased (-1.7%) to 96.417% when pulling fcf87d011c3245b92a67957c28316967547c1ed6 on draft_statements into 9190a3e8e92acc9fb73b678136126742f79232c7 on master.

[coveralls]

Coverage decreased (-1.6%) to 96.523% when pulling 17462a52150c208369da57433f61eb5b0a4bcaf4 on draft_statements into 9190a3e8e92acc9fb73b678136126742f79232c7 on master.

[coveralls]

Coverage decreased (-1.6%) to 96.523% when pulling 83c1e2ca20f46745df98fbcf4bb787fe6f4e84b1 on draft_statements into 9190a3e8e92acc9fb73b678136126742f79232c7 on master.

[bloody76]

Review status: 0 of 20 files reviewed at latest revision, 42 unresolved discussions, some commit checks failed.

---

src/ast/block_statement.h, line 11 at r1 (raw file):

Previously, nitnelave (nitnelave) wrote…

It's ordered, so list is better

Done

---

src/parser/parser.cc, line 284 at r2 (raw file):

Previously, nitnelave (nitnelave) wrote…

No, I mean that EXPECT_TOKEN is a user error, whereas the current token not being `{` is a developer error.

Done.

---

tools/clang-format.sh, line 10 at r3 (raw file):

Previously, nitnelave (nitnelave) wrote…

maybe check if readlink exists, otherwise define a shell function readlink?

will do later, in another review

---

tools/clang-format.sh, line 14 at r3 (raw file):

Previously, nitnelave (nitnelave) wrote…

`.*\.(cc|h)`

Done.

---

Comments from Reviewable

[coveralls]

Coverage decreased (-1.6%) to 96.523% when pulling 67d3cbf41e67c0be6772263c9cc7fccd1e7bc6f9 on draft_statements into 9190a3e8e92acc9fb73b678136126742f79232c7 on master.

[coveralls]

Coverage decreased (-0.9%) to 97.263% when pulling 7dad60ead0ed4e79de7689a0dcefb2b995ec7b87 on draft_statements into 9190a3e8e92acc9fb73b678136126742f79232c7 on master.

[coveralls]

Coverage decreased (-0.9%) to 97.263% when pulling 0968ac35cd4821a198ece12962f6859d0c5709dc on draft_statements into 9190a3e8e92acc9fb73b678136126742f79232c7 on master.

[coveralls]

Coverage increased (+0.4%) to 98.529% when pulling be8804e09635bd93bbb403cff00eccde323b86dc on draft_statements into 9190a3e8e92acc9fb73b678136126742f79232c7 on master.

[nitnelave]

Reviewed 2 of 17 files at r3, 2 of 20 files at r4, 29 of 32 files at r5. Review status: 29 of 33 files reviewed at latest revision, 48 unresolved discussions.

---

src/ast/builtin_type.cc, line 42 at r5 (raw file):

  }

  return nullptr;

Not needed, we're switching on an enum, the compiler will tell us if we're missing a case. This is dead code. And I don't want any nullptr in the code, all pointers are valid, and if you want nullptr, you use Option<T*>

---

src/error/error.h, line 91 at r2 (raw file):

Previously, bloody76 (Bd76) wrote…

I have the version 4.0.1, should I downgrade ?

There are still whitespace differences, how come?

---

src/parser/parser.cc, line 31 at r2 (raw file):

Previously, nitnelave (nitnelave) wrote…

I would put the grammar in documentation at the header level. Implementation level comments usually go inside the function, to document a specific implementation detail.

Header?

---

src/parser/parser.cc, line 18 at r5 (raw file):

#include <iostream>

#define ASSERT_TOKEN(TYPE)                          \

No, I'm still not a fan. I'd rather have the program crash in debug mode than have a parsing error. It's an invariant that the code must respect.

---

test/resources/resources.cc, line 35 at r5 (raw file):

    return true;
  }
  auto start = line.find_first_of('^');

I want to make sure that there's only whitespace on the line apart from "^"

---

test/resources/ir/simple_return_function.ref, line 1 at r5 (raw file):

define i32 @test() {

hmmm... This function should return void... Well, I'm working on typechecking, so I guess that's my responsibility, but I would have liked to avoid adding wrong tests.

---

test/resources/parser/no_statement.gh, line 4 at r5 (raw file):

  Int;
//^^^
// ERROR: Expected value identifier

I disagree with the error message, though. I would catch the fact that parsing a value failed (at the statement level), and return "Expected statement" or something.

---

test/resources/pretty_printer/value_statement.gh, line 2 at r5 (raw file):

fun test() {
    4;

The 4 spaces are just for fun? :p

---

Comments from Reviewable

[bloody76]

src/ast/builtin_type.cc, line 42 at r5 (raw file):

Previously, nitnelave (nitnelave) wrote…

Not needed, we're switching on an enum, the compiler will tell us if we're missing a case. This is dead code. And I don't want any nullptr in the code, all pointers are valid, and if you want nullptr, you use `Option`

I added this because the compiler told me there was possible missing cases.

---

Comments from Reviewable

[bloody76]

Review status: 29 of 33 files reviewed at latest revision, 22 unresolved discussions.

---

src/error/error.h, line 91 at r2 (raw file):

Previously, nitnelave (nitnelave) wrote…

There are still whitespace differences, how come?

You are using the 3.8 version ? Or 3.9 ?

clang-format version 3.8.0-2ubuntu4 (tags/RELEASE_380/final)```

---

src/parser/parser.cc, line 18 at r5 (raw file):

Previously, nitnelave (nitnelave) wrote…

No, I'm still not a fan. I'd rather have the program crash in debug mode than have a parsing error. It's an invariant that the code must respect.

Han ok, got it.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.4%) to 98.529% when pulling 7f5ce55e55d3ebf27a25eb6c00e3eefff22d942c on draft_statements into 9190a3e8e92acc9fb73b678136126742f79232c7 on master.

[bloody76]

Review status: 29 of 33 files reviewed at latest revision, 24 unresolved discussions.

---

test/resources/resources.cc, line 35 at r5 (raw file):

Previously, nitnelave (nitnelave) wrote…

I want to make sure that there's only whitespace on the line apart from "^"

I will add a check if you want.

---

test/resources/ir/simple_return_function.ref, line 1 at r5 (raw file):

Previously, nitnelave (nitnelave) wrote…

hmmm... This function should return void... Well, I'm working on typechecking, so I guess that's my responsibility, but I would have liked to avoid adding wrong tests.

Indeed, I propose we update the test once we got some types associated.

---

test/resources/parser/no_statement.gh, line 4 at r5 (raw file):

Previously, nitnelave (nitnelave) wrote…

I disagree with the error message, though. I would catch the fact that parsing a value failed (at the statement level), and return "Expected statement" or something.

I see, agreed. Would be more relevant.

---

test/resources/pretty_printer/value_statement.gh, line 2 at r5 (raw file):

Previously, nitnelave (nitnelave) wrote…

The 4 spaces are just for fun? :p

vim config :P

---

Comments from Reviewable

[nitnelave]

Review status: 29 of 33 files reviewed at latest revision, 22 unresolved discussions.

---

src/ast/builtin_type.cc, line 42 at r5 (raw file):

Previously, bloody76 (Bd76) wrote…

I added this because the compiler told me there was possible missing cases.

Really? Which compiler?

---

Comments from Reviewable

[nitnelave]

Review status: 29 of 33 files reviewed at latest revision, 22 unresolved discussions.

---

src/error/error.h, line 91 at r2 (raw file):

Previously, bloody76 (Bd76) wrote…

You are using the 3.8 version ? Or 3.9 ? ```$ clang-format-3.8 --version clang-format version 3.8.0-2ubuntu4 (tags/RELEASE_380/final)```

Try to reset to the previous version, apparently both are acceptable reformatting, but clang-format won't convert from one to the other.

---

Comments from Reviewable

[nitnelave]

Review status: 29 of 33 files reviewed at latest revision, 22 unresolved discussions.

---

src/ast/builtin_type.cc, line 42 at r5 (raw file):

Previously, nitnelave (nitnelave) wrote…

Really? Which compiler?

Wait what? In your branch I do get there, that's not normal. Oh, you added a test with UnknownTypeWidth. Why? It's not supposed to be run. If you want to have something, add an assert(false && "Unreachable code");. And remove the test.

---

Comments from Reviewable

[bloody76]

Review status: 29 of 33 files reviewed at latest revision, 22 unresolved discussions.

---

src/ast/builtin_type.cc, line 42 at r5 (raw file):

Previously, nitnelave (nitnelave) wrote…

Really? Which compiler?

clang version 3.8.0-2ubuntu4 (tags/RELEASE_380/final)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin```

---

test/resources/pretty_printer/value_statement.gh, line 2 at r5 (raw file):

Previously, bloody76 (Bd76) wrote…

vim config :P

Done.

---

Comments from Reviewable

[nitnelave]

Review status: 26 of 32 files reviewed at latest revision, 20 unresolved discussions.

---

src/ast/builtin_type.cc, line 42 at r5 (raw file):

Previously, bloody76 (Bd76) wrote…

```$ clang-3.8 --version clang version 3.8.0-2ubuntu4 (tags/RELEASE_380/final) Target: x86_64-pc-linux-gnu Thread model: posix InstalledDir: /usr/bin```

With clang version 3.8.1-24 (tags/RELEASE_381/final) I don't have any warning...

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.4%) to 98.529% when pulling 76930b83251df722c2590cbf9eaff798f36027eb on draft_statements into 9190a3e8e92acc9fb73b678136126742f79232c7 on master.

[coveralls]

Coverage increased (+0.4%) to 98.529% when pulling 76930b83251df722c2590cbf9eaff798f36027eb on draft_statements into 9190a3e8e92acc9fb73b678136126742f79232c7 on master.

[nitnelave]

Review status: 26 of 31 files reviewed at latest revision, 23 unresolved discussions.

---

src/ast/builtin_type.cc, line 28 at r7 (raw file):

  if (type == &int8) return IntWidth::W_8;

  assert(false && "Unreachable code");

Ah, no, this one is not unreachable.

---

src/parser/parser.cc, line 1 at r7 (raw file):

/// Grammar:

I thought it would be better to have the grammar that each function parses at the level of the function. Otherwise, it's harder to maintain. The overall grammar should be kept out of the code, I think.

But a comment here to introduce notations like <>, (), ?, *, would be good.

---

test/resources/resources.cc, line 39 at r7 (raw file):

  if (start == std::string::npos) return false;

  range.begin = {lineno - 1, static_cast<int>(start) + 1};

Are you sure of this static_cast? Shouldn't it be like std::distance(line.begin(), start); or something?

---

test/ast/builtin_type.cc, line 12 at r5 (raw file):

TEST(BuiltinType, UnknownIntType) {
  auto should_be_none = ast::types::int_type_to_width(nullptr);

This test is fine, the other one is not.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.4%) to 98.529% when pulling 096380f82c1a89c6e1fb1b9ee6b64eb0d66dcd53 on draft_statements into 9190a3e8e92acc9fb73b678136126742f79232c7 on master.

[bloody76]

Review status: 26 of 31 files reviewed at latest revision, 23 unresolved discussions, some commit checks failed.

---

src/ast/builtin_type.cc, line 28 at r7 (raw file):

Previously, nitnelave (nitnelave) wrote…

Ah, no, this one is not unreachable.

Ha ok, mistunderstood.

---

src/parser/parser.cc, line 1 at r7 (raw file):

Previously, nitnelave (nitnelave) wrote…

I thought it would be better to have the grammar that each function parses at the level of the function. Otherwise, it's harder to maintain. The overall grammar should be kept out of the code, I think. But a comment here to introduce notations like <>, (), ?, *, would be good.

I don't get it, that's what we had before ?

---

test/resources/resources.cc, line 39 at r7 (raw file):

Previously, nitnelave (nitnelave) wrote…

Are you sure of this static_cast? Shouldn't it be like `std::distance(line.begin(), start);` or something?

yes it returns the index in the string. It's a std::size_t behind, not an iterator

---

Comments from Reviewable