Support argument declaration for functions

[bloody76]

This change is 

[coveralls]

Coverage increased (+0.3%) to 98.79% when pulling c5901ef48e19b7751da58b30b7e5e7232f6e4213 on support_fun_args_declaration into a60aa1d02ade49934ef4c96de17becf1c60e6d43 on master.

[coveralls]

Coverage increased (+0.3%) to 98.79% when pulling eea90e62404afe83ff3a9470215e365c4b9cbea2 on support_fun_args_declaration into a60aa1d02ade49934ef4c96de17becf1c60e6d43 on master.

[nitnelave]

Reviewed 20 of 20 files at r1. Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

src/codegen/codegen_function.cc, line 47 at r1 (raw file):

  llvm_function->setCallingConv(CallingConv::C);

  // Name the parameters.

Add an assert that the two lists are the same size. And you don't need std:: for begin and end, because of argument-dependent lookup.

---

test/resources/ir/function_with_arg.gh, line 1 at r1 (raw file):

fun main(a: Int) {

Int32, until we have type aliases

---

test/resources/ir/function_with_args.gh, line 1 at r1 (raw file):

fun main(a: Int, b: Int) {

I'm not super fond of calling them main, since we'll eventually enforce a signature for main

---

Comments from Reviewable

[bloody76]

Review status: 6 of 20 files reviewed at latest revision, 3 unresolved discussions.

---

test/resources/ir/function_with_args.gh, line 1 at r1 (raw file):

Previously, nitnelave (nitnelave) wrote…

I'm not super fond of calling them `main`, since we'll eventually enforce a signature for main

Done.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.3%) to 98.79% when pulling ca391f7f27781ac38445b5e0ed31e0b04085fac8 on support_fun_args_declaration into d233c8f758da41926cb2b1a8b9bfd416291fe35b on master.

[bloody76]

Review status: 6 of 20 files reviewed at latest revision, 3 unresolved discussions.

---

test/resources/ir/function_with_arg.gh, line 1 at r1 (raw file):

Previously, nitnelave (nitnelave) wrote…

Int32, until we have type aliases

Done.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.3%) to 98.79% when pulling 6a01dd8d0a1ac0d479786f2818f8f57d7fe5f5dc on support_fun_args_declaration into d233c8f758da41926cb2b1a8b9bfd416291fe35b on master.

[bloody76]

Review status: 5 of 20 files reviewed at latest revision, 3 unresolved discussions.

---

src/codegen/codegen_function.cc, line 47 at r1 (raw file):

Previously, nitnelave (nitnelave) wrote…

Add an assert that the two lists are the same size. And you don't need `std::` for begin and end, because of argument-dependent lookup.

Done.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.3%) to 98.79% when pulling 723c1b7f09a98a3e5abf12c7080a3a85887deb62 on support_fun_args_declaration into d233c8f758da41926cb2b1a8b9bfd416291fe35b on master.

[nitnelave]

Sorry, how did I miss that? The syntax has a val or mut: fun test(val a : Int32)

---

Review status: 5 of 20 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[bloody76]

Done !

---

Review status: 2 of 21 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.3%) to 98.802% when pulling 10bac0df2a8028ead9ea7cf69f2babbbb39c8dc7 on support_fun_args_declaration into 116f6f7ffdfb4d73a8d96a0f6b683282365fea36 on master.

[coveralls]

Coverage increased (+0.3%) to 98.807% when pulling 10bac0df2a8028ead9ea7cf69f2babbbb39c8dc7 on support_fun_args_declaration into 116f6f7ffdfb4d73a8d96a0f6b683282365fea36 on master.

[nitnelave]

Reviewed 2 of 20 files at r2. Review status: 4 of 22 files reviewed at latest revision, 4 unresolved discussions.

---

src/ast/function_argument_declaration.h, line 13 at r2 (raw file):

  FunctionArgumentDeclaration(lexer::Range location, Identifier id, Type type,
                              bool mut)
      : ASTNode(std::move(location)),

Why not have it be a variable declaration? or contain one? Or extract the common part into an abstract class? There's much overlap here (intentionally, I might add)

---

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

  if (!value.is_ok() && !type.is_ok()) {
    return ParseError("Expected either a type, either a value to infer from",

either/or, not either/either. And no comma: "Expected either a type or a default value to infer from"

---

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

    arguments.push_back(std::make_unique<ast::FunctionArgumentDeclaration>(
        location.range(), val_decl->id(),
        std::move(val_decl->type().value_or_die()), val_decl->is_mutable()));

You don't know if there is a type, you can't use value_or_die()

---

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

FORMAT_CMD="clang-format -style=Google"

FIND_CMD="find ${PROJECT_DIR}/src/ ${PROJECT_DIR}/test/ -regex '.*\.[hc]\{1,2\}' -type f"

Will have to rebase after PR #44 is merged

---

Comments from Reviewable

[bloody76]

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

---

src/ast/function_argument_declaration.h, line 13 at r2 (raw file):

Previously, nitnelave (nitnelave) wrote…

Why not have it be a variable declaration? or contain one? Or extract the common part into an abstract class? There's much overlap here (intentionally, I might add)

It would be much better indeed, I just have issues regarding the pretty printer, I will give it another try, maybe last time I was tired.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.3%) to 98.806% when pulling 53817525f675d8bb561bb36d4ba3d57bd059918d on support_fun_args_declaration into 116f6f7ffdfb4d73a8d96a0f6b683282365fea36 on master.

[bloody76]

Review status: 0 of 32 files reviewed at latest revision, 4 unresolved discussions.

---

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

Previously, nitnelave (nitnelave) wrote…

either/or, not either/either. And no comma: "Expected either a type or a default value to infer from"

Done.

---

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

Previously, nitnelave (nitnelave) wrote…

You don't know if there is a type, you can't use value_or_die()

Done.

---

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

Previously, nitnelave (nitnelave) wrote…

Will have to rebase after PR #44 is merged

Yup

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.3%) to 98.806% when pulling 8baba116f9f55aaa4a33a5c7db585726df59f55e on support_fun_args_declaration into 116f6f7ffdfb4d73a8d96a0f6b683282365fea36 on master.

[coveralls]

Coverage increased (+0.3%) to 98.806% when pulling 8baba116f9f55aaa4a33a5c7db585726df59f55e on support_fun_args_declaration into 116f6f7ffdfb4d73a8d96a0f6b683282365fea36 on master.

[nitnelave]

Review status: 0 of 31 files reviewed at latest revision, 6 unresolved discussions.

---

src/ast/function_argument_declaration.h, line 16 at r3 (raw file):

                              Option<std::unique_ptr<Value>> value, bool mut)
      : VariableDeclaration(std::move(location), std::move(id), std::move(type),
                            std::move(value), mut) {}

I would put an assert that we have at least either a type or a value.

---

src/parser/parser.cc, line 201 at r3 (raw file):

template <class Declaration>
Parser::ErrorOrPtr<Declaration> Parser::parse_variable_declaration() {
  static_assert(std::is_base_of<ast::VariableDeclaration, Declaration>::value,

I like that :)

---

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

Previously, bloody76 (Bd76) wrote…

Yup

Meanwhile, can you remove this change from the PR?

---

Comments from Reviewable

[bloody76]

Review status: 0 of 31 files reviewed at latest revision, 5 unresolved discussions.

---

src/ast/function_argument_declaration.h, line 16 at r3 (raw file):

Previously, nitnelave (nitnelave) wrote…

I would put an assert that we have at least either a type or a value.

Indeed, I put it at the parse_variable_declaration level but I will do it here also !

---

Comments from Reviewable

[bloody76]

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

---

src/ast/function_argument_declaration.h, line 16 at r3 (raw file):

Previously, bloody76 (Bd76) wrote…

Indeed, I put it at the parse_variable_declaration level but I will do it here also !

Done.

---

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

Previously, nitnelave (nitnelave) wrote…

Meanwhile, can you remove this change from the PR?

Done.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.3%) to 98.65% when pulling 48227f5a7a4108f75ad319fb858d2ad9e72fad2c on support_fun_args_declaration into 1678e264d891f3c0e1f0ba95e11860b8a450a00b on master.

[nitnelave]

Review status: 0 of 30 files reviewed at latest revision, 1 unresolved discussion.

---

src/ast/function_argument_declaration.h, line 16 at r3 (raw file):

Previously, bloody76 (Bd76) wrote…

Done.

Erm, I don't see it...

---

Comments from Reviewable

[bloody76]

Review status: 0 of 30 files reviewed at latest revision, 1 unresolved discussion.

---

src/ast/function_argument_declaration.h, line 16 at r3 (raw file):

Previously, nitnelave (nitnelave) wrote…

Erm, I don't see it...

its in variable_declaration.h, this way we assert for all the kind of variable declaration

---

Comments from Reviewable

[nitnelave]

---

Review status: 0 of 30 files reviewed at latest revision, 1 unresolved discussion.

---

src/ast/function_argument_declaration.h, line 16 at r3 (raw file):

Previously, bloody76 (Bd76) wrote…

its in variable_declaration.h, this way we assert for all the kind of variable declaration

OK, Got it

---

Comments from Reviewable