Skip to content
This repository was archived by the owner on Nov 6, 2020. It is now read-only.

Removing match on constant #888

Merged
merged 3 commits into from
Apr 9, 2016
Merged

Removing match on constant #888

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
405a6bf
Removing match on constant
Apr 6, 2016
0ef6de9
Update account.rs
gavofyork Apr 9, 2016
04d5b5c
Merge branch 'master' into h256
Apr 9, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ethcore/src/account.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ impl Account {
/// get someone who knows to call `note_code`.
pub fn code(&self) -> Option<&[u8]> {
match self.code_hash {
Some(SHA3_EMPTY) | None if self.code_cache.is_empty() => Some(&self.code_cache),
Some(c) if c == SHA3_EMPTY && self.code_cache.is_empty() => Some(&self.code_cache),
Copy link
Contributor

@gavofyork gavofyork Apr 9, 2016

Choose a reason for hiding this comment

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

extra space.

Some(_) if !self.code_cache.is_empty() => Some(&self.code_cache),
None => Some(&self.code_cache),
Copy link
Collaborator Author

@tomusdrw tomusdrw Apr 6, 2016

Choose a reason for hiding this comment

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

Does this actually make sense? Before this refactor we were matching None twice:

None if self.code_cache.is_empty() => Some(&self.code_cache),
None => Some(&self.code_cache),

Copy link
Collaborator

@arkpar arkpar Apr 6, 2016

Choose a reason for hiding this comment

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

Looks like it does not. None matches to Some(&self.code_cache) in all cases
@gavofyork please take a look, might be a bug in the original code.

Copy link
Contributor

@gavofyork gavofyork Apr 8, 2016

Choose a reason for hiding this comment

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

the original code (https://github.com/ethcore/parity/blob/master/ethcore/src/account.rs) was:

Some(c) if c == SHA3_EMPTY  && self.code_cache.is_empty() => Some(&self.code_cache),
Some(_) if !self.code_cache.is_empty() => Some(&self.code_cache),
None => Some(&self.code_cache),
_ => None,

there are no options here which are superfluous.

Copy link
Collaborator Author

@tomusdrw tomusdrw Apr 8, 2016

Choose a reason for hiding this comment

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

The code from master already contains this change (was included with #889 - otherwise there was compilation error on nightly).
This is what it looked like before:
https://github.com/ethcore/parity/blob/61420d3c9c64d87ab2c83d972c7d2d812b1d9b72/ethcore/src/account.rs#L141

The logic wasn't different, but there was superfluous | None in first branch.
My question is rather if when this method should return None - currently it's only the case when code_hash == SHA3_EMPTY && !code_cache.is_empty() OR code_hash != SHA3_EMPTY && code_cache.is_empty() - this is correct?

Copy link
Contributor

@gavofyork gavofyork Apr 9, 2016

Choose a reason for hiding this comment

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

Correct (technically, the first is actually a logic error and should properly be either asserted against or an error). The second (i.e. there is definitely some non-empty code, but it isn't presently cached) properly returns None.

_ => None,
Expand Down