Deprecate AEVM #866

2022-04-12T21:26:31+09:00

zxq9 commented

2022-04-12 21:26:31 +09:00

(Migrated from gitlab.com)

Created by: radrow

fixes #226

Removed AEVM backend
Removed all tests for AEVM
Rewritten some tests to actually test FATE
Removed unused functions
Removed unmentioned tests

*Created by: radrow* fixes #226 - Removed AEVM backend - Removed all tests for AEVM - Rewritten some tests to actually test FATE - Removed unused functions - Removed unmentioned tests

ghallak (Migrated from gitlab.com) approved these changes 2022-04-12 21:26:31 +09:00

ghallak commented

2022-04-18 23:24:10 +09:00

(Migrated from gitlab.com)

Why have you decided to export type_to_fcode/2 even though it's not used anywhere outside of aeso_ast_to_fcode.erl?

Why have you decided to export `type_to_fcode/2` even though it's not used anywhere outside of `aeso_ast_to_fcode.erl`?

ghallak commented

2022-04-19 00:07:11 +09:00

(Migrated from gitlab.com)

I don't see this error being thrown anywhere in the code. And I would say the same for the error last_declaration_must_be_main_contract , it's also never used anywhere in the code.

I don't see this error being thrown anywhere in the code. And I would say the same for the error `last_declaration_must_be_main_contract `, it's also never used anywhere in the code.

ghallak commented

2022-04-19 00:28:46 +09:00

(Migrated from gitlab.com)

The function pp_bytecode was removed, do you still need that? There is also a reference for pp_bytecode in docs/aeso_compiler.md in case you want to remove this.

The function `pp_bytecode` was removed, do you still need that? There is also a reference for `pp_bytecode` in `docs/aeso_compiler.md` in case you want to remove this.

ghallak commented

2022-04-19 00:33:10 +09:00

(Migrated from gitlab.com)

Why do you need to export type_to_scode/1?

Why do you need to export `type_to_scode/1`?

ghallak commented

2022-04-19 00:37:38 +09:00

(Migrated from gitlab.com)

Since there are no non-compatible contracts now, would it make sense to have test all contracts inside test/contracts instead of listing all contracts names in the function compilable_contracts?

Since there are no non-compatible contracts now, would it make sense to have test all contracts inside `test/contracts` instead of listing all contracts names in the function `compilable_contracts`?

ghallak commented

2022-04-19 00:44:14 +09:00

(Migrated from gitlab.com)

Review: Changes requested

Beside the comments, there are a couple of things I think should be done:

Grep for sophia_type_to_typerep and remove the reference to that from the docs.
Grep for AEVM and remove the references to that from the docs and from test/contracts/code_errors/unapplied_named_arg_builtin.aes and test/contracts/unapplied_builtins.aes.

**Review:** Changes requested Beside the comments, there are a couple of things I think should be done: * Grep for `sophia_type_to_typerep` and remove the reference to that from the docs. * Grep for `AEVM` and remove the references to that from the docs and from `test/contracts/code_errors/unapplied_named_arg_builtin.aes` and `test/contracts/unapplied_builtins.aes`.

zxq9 commented

2022-04-20 02:21:30 +09:00

(Migrated from gitlab.com)

Created by: radrow

-export([ast_to_fcode/2, format_fexpr/1]).

*Created by: radrow* ```suggestion:-0+0 -export([ast_to_fcode/2, format_fexpr/1]). ```

zxq9 commented

2022-04-20 02:23:08 +09:00

(Migrated from gitlab.com)

Created by: radrow

No, there are contracts that don't compile for tests that check compiler errors

*Created by: radrow* No, there are contracts that don't compile for tests that check compiler errors

zxq9 commented

2022-04-20 02:24:13 +09:00

(Migrated from gitlab.com)

Created by: radrow

-export([compile/2, compile/3, term_to_fate/1, term_to_fate/2]).

*Created by: radrow* ```suggestion:-0+0 -export([compile/2, compile/3, term_to_fate/1, term_to_fate/2]). ```

zxq9 commented

2022-04-20 02:42:11 +09:00

(Migrated from gitlab.com)

Created by: radrow

TODO:
[X] check error messages in code_errors if all are needed
[X] check for orphaned tests

(done)

*Created by: radrow* TODO: [X] check error messages in code_errors if all are needed [X] check for orphaned tests (done)

ghallak commented

2022-04-28 21:05:53 +09:00

(Migrated from gitlab.com)

I have no idea if this is related, but I'll ask anyway.

What is fun_clauses? Is it something that should be deprecated? There are no parsing rule for it in aeso_parser.erl and I don't see how it can be derived from any thing else.

I have no idea if this is related, but I'll ask anyway. What is `fun_clauses`? Is it something that should be deprecated? There are no parsing rule for it in `aeso_parser.erl` and I don't see how it can be derived from any thing else.

zxq9 commented

2022-04-29 06:27:54 +09:00

(Migrated from gitlab.com)

Created by: radrow

I have no idea if this is related, but I'll ask anyway.

What is fun_clauses? Is it something that should be deprecated? There are no parsing rule for it in aeso_parser.erl and I don't see how it can be derived from any thing else.

They are used somewhere in the process of multi-clause function definitions resolution. In the end they turn into case expressions

*Created by: radrow* > I have no idea if this is related, but I'll ask anyway. > > What is `fun_clauses`? Is it something that should be deprecated? There are no parsing rule for it in `aeso_parser.erl` and I don't see how it can be derived from any thing else. They are used somewhere in the process of multi-clause function definitions resolution. In the end they turn into case expressions

ghallak commented

2022-05-05 16:16:39 +09:00

(Migrated from gitlab.com)

How was this working before you made this change? Apparently there was no handling of last_declaration_must_be_contract_def error. Shouldn't be a test somewhere for that?

How was this working before you made this change? Apparently there was no handling of `last_declaration_must_be_contract_def` error. Shouldn't be a test somewhere for that?

ghallak commented

2022-05-05 16:21:44 +09:00

(Migrated from gitlab.com)

Right, I missed that.

ghallak commented

2022-05-05 16:50:48 +09:00

(Migrated from gitlab.com)

What else is missing for this PR?

zxq9 commented

2022-05-10 18:56:31 +09:00

(Migrated from gitlab.com)

Created by: radrow

This is an fcode generation error, currently it is impossible for a user to cause it. It is left for cases where someone pins in the middle of the compilation pipeline, or something goes bad in the typechecking process

*Created by: radrow* This is an fcode generation error, currently it is impossible for a user to cause it. It is left for cases where someone pins in the middle of the compilation pipeline, or something goes bad in the typechecking process

zxq9 commented

2022-05-10 22:10:32 +09:00

(Migrated from gitlab.com)

Created by: hanssv

🙈 Nothing wrong with the original code, this is neither shorter nor more readable?

*Created by: hanssv* 🙈 Nothing wrong with the original code, this is neither shorter nor more readable?

zxq9 commented

2022-05-10 22:15:11 +09:00

(Migrated from gitlab.com)

Created by: hanssv

Review: Commented

Looks good, great to finally get rid of the AEVM!

One weird change that I'd prefer to see un-done 🙏

*Created by: hanssv* **Review:** Commented Looks good, great to finally get rid of the AEVM! One weird change that I'd prefer to see un-done 🙏

zxq9 commented

2022-05-10 22:31:50 +09:00

(Migrated from gitlab.com)

Created by: hanssv

Review: Approved

👍

*Created by: hanssv* **Review:** Approved 👍

ghallak commented

2022-05-10 22:32:32 +09:00

(Migrated from gitlab.com)

approved this merge request

zxq9 commented

2022-05-10 22:33:59 +09:00

(Migrated from gitlab.com)

Merged by: radrow at 2022-05-10 13:33:59 UTC

*Merged by: radrow at 2022-05-10 13:33:59 UTC*

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: QPQ-AG/sophia#866