Parser: basic function declaration

[nitnelave]

This change is 

[coveralls]

Coverage increased (+1.7%) to 95.646% when pulling 21ccb1131b30d0fd4ca0edb24ce8554251a4192d on parser into 19d7b2038f16b72ddf97af2f985c92f0b62195d2 on master.

[Brolock]

Review status: 0 of 42 files reviewed at latest revision, 9 unresolved discussions.

---

src/lexer/token.h, line 245 at r2 (raw file):

    if (value().is<std::string>()) return text();
    if (value().is<int64_t>()) return std::to_string(int_value());
    assert(false);

That's not a nice assert message

---

src/lexer/token.h, line 250 at r2 (raw file):

  std::string to_string() const {
    std::stringstream ss;
    ss << "{" << type();

Shouldn't it be something like type().to_string() ? Since type is an int I don't know if you want to print "{9} at location" in case of empty string?

---

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

ErrorOr Parser::parse_type_identifier(bool simple) {

ErrorOr Parser::parse_value_identifier(bool simple) {

Have the same body except for the ParseError returned, maybe having a 3rd function called by both to avoid code duplication

---

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

    } else {
      return Identifier(text.str(), location.range(), true, absolute);
      ;

Uh?

---

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

    RETURN_IF_ERROR(get_token());
    return Identifier(text.str(), location.range(), false, absolute);
    ;

I guess there is something with those ; Do you mind to enlighten me?

---

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

  assert(current_token().type() == TokenType::VAL ||
         current_token().type() == TokenType::MUT);
  bool mut = current_token().type() == TokenType::MUT;

mut declared here and only used on return statement

---

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

    : parser_(parser), position_(parser_->current_token().location().begin) {}

lexer::Range ScopedLocation::range() const {

Why would you want to return a non-const Range? For what I saw you only look at the range and never change it

---

test/test_utils/lexing.cc, line 12 at r2 (raw file):

    result.emplace_back(std::move(tok));
  }
  return std::move(result);

Isn't RVO supposed to do that instead of you?

---

tools/lcov.sh, line 9 at r2 (raw file):

lcov --gcov-tool "${DIRNAME}/llvm-gcov" \
  --rc lcov_branch_coverage=1 \
  --rc lcov_excl_line="(^[-0-9: #]*};?$)|(LCOV_EXCL_LINE)|(assert\(false)" \

I don't know what that is but I see assert(false) I comment

---

Comments from Reviewable

[nitnelave]

Review status: 0 of 42 files reviewed at latest revision, 9 unresolved discussions.

---

src/lexer/token.h, line 245 at r2 (raw file):

Previously, Brolock (Marcelin Dupraz) wrote…

That's not a nice assert message

Told it to straighten up and be nicer.

---

src/lexer/token.h, line 250 at r2 (raw file):

Previously, Brolock (Marcelin Dupraz) wrote…

Shouldn't it be something like type().to_string() ? Since type is an int I don't know if you want to print "{9} at location" in case of empty string?

Operator << is overloaded for TokenType.

---

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

Previously, Brolock (Marcelin Dupraz) wrote…

> ErrorOr Parser::parse_type_identifier(bool simple) { > ErrorOr Parser::parse_value_identifier(bool simple) { Have the same body except for the ParseError returned, maybe having a 3rd function called by both to avoid code duplication

There's a slight difference: a boolean is negated in the condition. I already introduced parser_identifier for that purpose, but made it usable in more places. I think the code isn't worth factoring further with another function that would have a weird xor in the middle.

---

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

Previously, Brolock (Marcelin Dupraz) wrote…

Uh?

What, you don't like spaces? :p Fixed

---

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

Previously, Brolock (Marcelin Dupraz) wrote…

I guess there is something with those ; Do you mind to enlighten me?

All praise the copy pasta!

---

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

Previously, Brolock (Marcelin Dupraz) wrote…

mut declared here and only used on return statement

Yes, but the value of current_token() changes.

---

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

Previously, Brolock (Marcelin Dupraz) wrote…

Why would you want to return a non-const Range? For what I saw you only look at the range and never change it

I create a range, if they want to change it, good for them, it's theirs (cf std::move, for instance). In other words, why would I want it const?

---

test/test_utils/lexing.cc, line 12 at r2 (raw file):

Previously, Brolock (Marcelin Dupraz) wrote…

Isn't RVO supposed to do that instead of you?

Not in that case. Since we don't have a copy constructor, the code doesn't compile before being optimized into an RVO

---

tools/lcov.sh, line 9 at r2 (raw file):

Previously, Brolock (Marcelin Dupraz) wrote…

I don't know what that is but I see assert(false) I comment

exclude assert(false) lines from coverage.

---

Comments from Reviewable

[coveralls]

Coverage increased (+1.7%) to 95.626% when pulling 98134214d4cba37148d96005e0f7b3aa63e713b4 on parser into 19d7b2038f16b72ddf97af2f985c92f0b62195d2 on master.

[coveralls]

Coverage increased (+1.7%) to 95.672% when pulling 9830f45b7488f41afe2d078d6ec1c9c0452f1117 on parser into 19d7b2038f16b72ddf97af2f985c92f0b62195d2 on master.

[coveralls]

Coverage increased (+1.7%) to 95.672% when pulling b2da6b97d66d9ed44a2c73a5b98730d2ee9824f8 on parser into 19d7b2038f16b72ddf97af2f985c92f0b62195d2 on master.

[nitnelave]

Review status: 0 of 46 files reviewed at latest revision, 10 unresolved discussions.

---

src/lexer/lexer.cc, line 53 at r5 (raw file):

Previously, codacy-bot (Codacy Bot) wrote…

 Issue found: [Class 'LexerHelper' has a constructor with 1 argument that is not explicit.](https://www.codacy.com/app/nitnelave1/hopper/pullRequest?prid=766374)

Done.

---

Comments from Reviewable

[coveralls]

Coverage increased (+1.7%) to 95.672% when pulling 3138af978fb47bb5b83228afeeed5084112038ca on parser into 19d7b2038f16b72ddf97af2f985c92f0b62195d2 on master.

[Brolock]

---

Review status: 0 of 46 files reviewed at latest revision, 3 unresolved discussions.

---

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

ood for them, it's theirs (cf std::move, for instance). In other words, why would I want it const?

I just don't like giving non const things to people when it's not supposed to change, but yeah who cares

---

test/test_utils/lexing.cc, line 12 at r2 (raw file):

Previously, nitnelave (nitnelave) wrote…

Not in that case. Since we don't have a copy constructor, the code doesn't compile before being optimized into an RVO

Oh, forgot about it

---

Comments from Reviewable