Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compile warnings #265

Merged
merged 4 commits into from
Jul 9, 2018
Merged

Compile warnings #265

merged 4 commits into from
Jul 9, 2018

Conversation

lazovicff
Copy link
Contributor

@lazovicff lazovicff commented Jul 5, 2018

Changes include:

  • Refactored Authority contract to avoid stack depth error
  • Removed unused imports
  • Fixed compile and solium warnings such as:
    • line length
    • formating of more than 5 function arguments
    • unused function arguments
    • etc.

There are solium warnings for experimental features, im not sure if its safe to remove these.
Solc warnings for external libraries will unfortunately stay: ethereum/solidity#2675

@lazovicff lazovicff self-assigned this Jul 5, 2018
@lazovicff lazovicff changed the title Maintenance/compile warnings Compile warnings Jul 5, 2018
@lazovicff lazovicff force-pushed the maintenance/compile-warnings branch 2 times, most recently from b1dbf18 to b293a83 Compare July 6, 2018 14:55
@elenadimitrova elenadimitrova self-requested a review July 9, 2018 07:59
Copy link
Contributor

@elenadimitrova elenadimitrova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some additional solium issues

572:26    warning    Code contains empty block                                                                   no-empty-blocks
  572:26    warning    An empty block shouldn't have any whitespace or comments between the braces, i.e., '{}'.    whitespace
  600:30    warning    Code contains empty block                                                                   no-empty-blocks
  600:30    warning    An empty block shouldn't have any whitespace or comments between the braces, i.e., '{}'.    whitespace

And also some remaining eslint issues mainly to do with console.log statements, which we should probably find a more generic way to handle since we have a lot.

function getImpliedRoot(bytes key, bytes value, uint branchMask, bytes32[] siblings) public view returns (bytes32) { // solium-disable-line security/no-assign-params
function getImpliedRoot(bytes key, bytes value, uint branchMask, bytes32[] siblings) public // solium-disable-line security/no-assign-params
view
returns (bytes32)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand we reach the line limit above but can we at least keep view returns (bytes32) on the same line?

function getExpectedSkillIdAndAddress( ReputationLogEntry storage logEntry, uint256 updateNumber ) internal returns (uint256 expectedSkillId, address expectedAddress) {
function getExpectedSkillIdAndAddress(ReputationLogEntry storage logEntry, uint256 updateNumber)
internal
view
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put internal view at the end of the function declaration line above. That's what we've followed everywhere else I think. The returns.. can be on its own line to stand out

@lazovicff lazovicff force-pushed the maintenance/compile-warnings branch from 33c01f3 to 9c91a61 Compare July 9, 2018 09:56
@elenadimitrova
Copy link
Contributor

Sorry, forgot to mention in review, to check if we can disable no-experimental in solium checks to avoid these warnings

@lazovicff lazovicff force-pushed the maintenance/compile-warnings branch from 9c91a61 to f11e30a Compare July 9, 2018 10:18
@lazovicff
Copy link
Contributor Author

I prefer suppressing these warnings by fixing them. In case where that is not possible i would leave them. But i guess we can supress them and be more cautious, no strong feelings

@elenadimitrova
Copy link
Contributor

You are not sufficiently informed when you say this, no hard feelings :)
To understand whether we need experimental pragmas you need to understand what they do, so read the documentation first before you decide to "fix" them. I want you to come back and tell me what each of the two experimental pragmas does and whether and why we need it.

Then check duaraghav8/Ethlint#201 where we've agreed this is a bad warning anyway that will be fixed in upcoming solium

@lazovicff
Copy link
Contributor Author

I agree that we should keep experimental pragmas, but my comment was about whether we should supress warnings (any warning, not just about experimental pragmas) by turning off the rule in config file.

You are right that i could do a research of what benefits of these experimental features give us. Looking from solidity releases i can see that ABIEncoderV2 allows us to use structs and arbitrary length arrays as functions arguments and return values. And v0.5.0 will disallow uninitialized storage pointers, old constructor sintax, var keywork etc.

@elenadimitrova
Copy link
Contributor

Yes, I was not arguing that we disable the rule to simply silence the warning. I was saying we should silence the rule because
a) we need those experimental pragmas so we can't remove like solium is asking
b) that solium rule is just wrong and will be removed in the next release anyway

@lazovicff lazovicff force-pushed the maintenance/compile-warnings branch from f785f9b to ac95269 Compare July 9, 2018 15:12
@elenadimitrova elenadimitrova merged commit 735cad0 into develop Jul 9, 2018
@elenadimitrova elenadimitrova deleted the maintenance/compile-warnings branch July 9, 2018 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants