Remove special yul strings from UnusedStoreEliminator #15337
Conversation
ekpyron
force-pushed
the
removeSpecialYulStrings
branch
3 times, most recently
from
59a72a3 to
ef00aef
Compare
ekpyron
force-pushed
the
removeSpecialYulStrings
branch
from
ef00aef to
33b818c
Compare
|
This pull request is stale because it has been open for 14 days with no activity. |
github-actions
bot
added
the
stale
ekpyron
removed
the
stale
|
This pull request is stale because it has been open for 14 days with no activity. |
github-actions
bot
added
the
stale
ekpyron
removed
the
stale
|
This pull request is stale because it has been open for 14 days with no activity. |
github-actions
bot
added
the
stale
clonker
removed
the
stale
|
This pull request is stale because it has been open for 14 days with no activity. |
github-actions
bot
added
the
stale
clonker
removed
the
stale
ekpyron
force-pushed
the
removeSpecialYulStrings
branch
from
33b818c to
9288e93
Compare
| std::set<Statement const*>& activeStorageStores() { return m_activeStores["s"_yulname]; } | ||
| std::set<Statement const*>& activeMemoryStores() { return m_activeStores[UnusedStoreEliminatorKey::Memory]; } | ||
| std::set<Statement const*>& activeStorageStores() { return m_activeStores[UnusedStoreEliminatorKey::Storage]; } | ||
| std::optional<u256> lengthValue(OperationLength const& _length) const |
There was a problem hiding this comment.
I'd give it a name more resembling the knowledge base function it replaces. lengthValue() sounds a bit plain and it might be too easy to not realize it's not doing something much simpler.
| std::optional<u256> lengthValue(OperationLength const& _length) const | |
| std::optional<u256> lengthValueIfKnownConstant(OperationLength const& _length) const |
There was a problem hiding this comment.
I think it's also fine like it is, the implementation is concise and right there - although I am not opposed to the longer name, either.
There was a problem hiding this comment.
When you read the code, you see the name, not the implementation and it should reflect important things about the function. IMO in this case it does not.
There was a problem hiding this comment.
We should also stop renaming and refactoring random stuff in PRs just because they happen to appear in the diff. It's not a new name introduced in the PR - if you're bothered by it, feel free to create a PR to rename it (even though I'd say the current name is fine), but it has nothing to do with what this PR does.
There was a problem hiding this comment.
oh damn, it actually is new :-D sorry, I didn't see it properly on my phone :-) (and thought this PR actually doesn't add anything :-))
There was a problem hiding this comment.
I'd still keep the name. The return type makes things pretty clear anyways
cameel
force-pushed
the
removeSpecialYulStrings
branch
from
9288e93 to
cbfe4a1
Compare
Just quickly written and not urgent - but before @clonker gets back to the actual YulName->ID replacement an FYI that the weird special names in the
UnusedStoreEliminatorcan easily be removed.