EOF: Refactor assemble functions in evmasm::Assembly
#15446
Conversation
|
Thank you for your contribution to the Solidity compiler! A team member will follow up shortly. If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother. If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix. |
rodiazet
force-pushed
the
eof-prep-4-refactor
branch
from
34b38fa to
774fda2
Compare
cameel
changed the title
assemble functions in evmasm::Assembly`assemble functions in evmasm::Assembly
There was a problem hiding this comment.
It looks good in terms of correctness - I haven't noticed any actual problems or bugs. There some very easy to fix cosmetic style issues like the missing _ or tab alignment that would be nice to fix before we merge.
Since you were thinking about the best way to refactor it, I also left some of my reflections on that. Note that some of them are mutually exclusive (e.g. no point defining types if you take the suggestion not to move the switches into dispatch functions), so just choose one. But also please don't take those as requirements for this to be approved - they're just FYI if you want to improve it a bit before we merge, but the main issue (duplication) is solved so I'm just going to go along with whatever you push next. I want to get this merged quickly so that we can proceed with the next PR.
rodiazet
force-pushed
the
eof-prep-4-refactor
branch
3 times, most recently
from
64aa730 to
2af37a1
Compare
rodiazet
changed the title
assemble functions in evmasm::Assemblyassemble functions in evmasm::Assembly
There was a problem hiding this comment.
Perfect.
I'd have merged it already but noticed some typos that I wanted to correct first. I wanted to just do it myself, but could not push a commit to your branch. I pushed a new branch instead: eof-prep-4-refactor. This solves the extra comments I left below. If you pull it into your branch and squash, we can merge.
| solRequire(_pos < 0xffffffffL, AssemblyException, "Tag too large."); | ||
| size_t tagId = static_cast<size_t>(_item.data()); | ||
| solRequire(m_tagPositionsInBytecode[tagId] == std::numeric_limits<size_t>::max(), AssemblyException, "Duplicate tag position."); | ||
| m_tagPositionsInBytecode[tagId] = _pos; |
There was a problem hiding this comment.
WTF, didn't even notice that we have that one defined as mutable :) I expected you to have to move it to m_tagPositionsInBytecode. But now I see that our assemble() is const too... Well, TBH I don't like how m_tagPositionsInBytecode works and I'd design it differently myself, but I want the PR merged, so probably no point messing with it now.
rodiazet
force-pushed
the
eof-prep-4-refactor
branch
from
2af37a1 to
ca82246
Compare
Done. You can merge. |
Co-authored-by: Kamil Śliwak <[email protected]>
rodiazet
force-pushed
the
eof-prep-4-refactor
branch
from
ca82246 to
545cab3
Compare
assembleLegacyandassebleEOFto be reused.switchs to corresponding functions.