hyperledger / solang

Solidity Compiler for Solana and Polkadot
https://solang.readthedocs.io/
Apache License 2.0
1.22k stars 207 forks source link

Duplicate code (part 1) #1261

Open LucasSte opened 1 year ago

LucasSte commented 1 year ago

My IDE points 320 places with repeated code. I am selecting the glaring ones and opening issues. Here are 12 of them (more to come in other issues).

  1. https://github.com/hyperledger/solang/blob/77d78369fafc2948f7ce26abcfff86edc18de144/src/sema/expression/assign.rs#L126-L137

https://github.com/hyperledger/solang/blob/77d78369fafc2948f7ce26abcfff86edc18de144/src/sema/expression/assign.rs#L371-L381

  1. https://github.com/hyperledger/solang/blob/77d78369fafc2948f7ce26abcfff86edc18de144/src/sema/expression/assign.rs#L205-L225

https://github.com/hyperledger/solang/blob/77d78369fafc2948f7ce26abcfff86edc18de144/src/sema/expression/arithmetic.rs#L927-L947

  1. https://github.com/hyperledger/solang/blob/77d78369fafc2948f7ce26abcfff86edc18de144/src/sema/expression/assign.rs#L350-L368

https://github.com/hyperledger/solang/blob/77d78369fafc2948f7ce26abcfff86edc18de144/src/sema/expression/assign.rs#L384-L402

4. https://github.com/hyperledger/solang/blob/77d78369fafc2948f7ce26abcfff86edc18de144/src/sema/ast.rs#L190-L197

https://github.com/hyperledger/solang/blob/77d78369fafc2948f7ce26abcfff86edc18de144/src/sema/ast.rs#L212-L217

https://github.com/hyperledger/solang/blob/77d78369fafc2948f7ce26abcfff86edc18de144/src/sema/ast.rs#L538-L543

================================================================

5. https://github.com/hyperledger/solang/blob/77d78369fafc2948f7ce26abcfff86edc18de144/src/sema/builtin.rs#L1490-L1514

https://github.com/hyperledger/solang/blob/77d78369fafc2948f7ce26abcfff86edc18de144/src/sema/builtin.rs#L1553-L1577

================================================================ 6. https://github.com/hyperledger/solang/blob/77d78369fafc2948f7ce26abcfff86edc18de144/src/codegen/cfg.rs#L1686-L1708

https://github.com/hyperledger/solang/blob/77d78369fafc2948f7ce26abcfff86edc18de144/src/codegen/cfg.rs#L1876-L1898

7. https://github.com/hyperledger/solang/blob/77d78369fafc2948f7ce26abcfff86edc18de144/src/codegen/constant_folding.rs#L1086-L1090

https://github.com/hyperledger/solang/blob/77d78369fafc2948f7ce26abcfff86edc18de144/src/codegen/constant_folding.rs#L1092-L1096

https://github.com/hyperledger/solang/blob/77d78369fafc2948f7ce26abcfff86edc18de144/src/codegen/constant_folding.rs#L1112-L1116

https://github.com/hyperledger/solang/blob/77d78369fafc2948f7ce26abcfff86edc18de144/src/codegen/constant_folding.rs#L1118-L1122

================================================================

8. https://github.com/hyperledger/solang/blob/77d78369fafc2948f7ce26abcfff86edc18de144/src/sema/expression/constructor.rs#L30-L49

https://github.com/hyperledger/solang/blob/77d78369fafc2948f7ce26abcfff86edc18de144/src/sema/expression/constructor.rs#L238-L257

9. https://github.com/hyperledger/solang/blob/77d78369fafc2948f7ce26abcfff86edc18de144/src/codegen/dead_storage.rs#L313-L321

https://github.com/hyperledger/solang/blob/77d78369fafc2948f7ce26abcfff86edc18de144/src/codegen/reaching_definitions.rs#L182-L190

10. https://github.com/hyperledger/solang/blob/77d78369fafc2948f7ce26abcfff86edc18de144/src/codegen/dead_storage.rs#L544-L554

https://github.com/hyperledger/solang/blob/77d78369fafc2948f7ce26abcfff86edc18de144/src/codegen/dead_storage.rs#L530-L541

11. https://github.com/hyperledger/solang/blob/77d78369fafc2948f7ce26abcfff86edc18de144/src/emit/expression.rs#L758-L786

https://github.com/hyperledger/solang/blob/77d78369fafc2948f7ce26abcfff86edc18de144/src/emit/expression.rs#L866-L894

https://github.com/hyperledger/solang/blob/77d78369fafc2948f7ce26abcfff86edc18de144/src/emit/expression.rs#L794-L823

https://github.com/hyperledger/solang/blob/77d78369fafc2948f7ce26abcfff86edc18de144/src/emit/expression.rs#L830-L858

12. https://github.com/hyperledger/solang/blob/77d78369fafc2948f7ce26abcfff86edc18de144/src/codegen/strength_reduce/expression_values.rs#L709-L719

https://github.com/hyperledger/solang/blob/77d78369fafc2948f7ce26abcfff86edc18de144/src/codegen/strength_reduce/expression_values.rs#L750-L760

xermicus commented 1 year ago

We should decide on a case by case basis. It is only sensible for cases where a de duplication a) removes LOC significantly and b) doesn't mess up the code just for the sake of DRY

bolajahmad commented 5 months ago

Is it still necessary to work on this refactor here? I know it's been open for a while but if it's still an issue, I can solve it.

I just need more information on what the expected action is. Do we need to just extract the code into a new function and replace the logic everywhere? How do I test to make sure code is working fine after my changes?

LucasSte commented 5 months ago

Hello @bolajahmad, We need to evaluate what is more suitable for each case. I haven't checked each location myself, but some cases require a function, others a Rust macro and others a refactoring. The best way is to read the code, encounter all the repeated fragments and choose one of these options.

In order to test your changes, you can run cargo test --workspace. Please, also follow the contributor guidelines if you wish to open a PR.