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

Prepare literalArguments for immutable builtin functions #8626

Merged
merged 1 commit into from Apr 9, 2020

Conversation

Marenz
Copy link
Contributor

@Marenz Marenz commented Apr 7, 2020

refs #8551

(incomplete)

@chriseth
Copy link
Contributor

chriseth commented Apr 7, 2020

This PR contains unrelated commits.

libyul/Dialect.h Outdated Show resolved Hide resolved
@Marenz Marenz force-pushed the immutable-functioncallgraph branch from a45070b to 68e08f3 Compare April 7, 2020 09:38
@Marenz Marenz force-pushed the immutable-functioncallgraph branch from 68e08f3 to c327b75 Compare April 7, 2020 12:39
@Marenz Marenz marked this pull request as ready for review April 7, 2020 12:39
@Marenz Marenz requested a review from chriseth April 7, 2020 13:14
@chriseth
Copy link
Contributor

chriseth commented Apr 7, 2020

Please squash.

libyul/Dialect.h Outdated Show resolved Hide resolved
libyul/AsmAnalysis.cpp Outdated Show resolved Hide resolved
@Marenz Marenz force-pushed the immutable-functioncallgraph branch 2 times, most recently from 05058ca to af8c3da Compare April 8, 2020 11:08
@Marenz
Copy link
Contributor Author

Marenz commented Apr 8, 2020

Pushed update that addresses the feedback

@@ -293,11 +293,13 @@ vector<YulString> AsmAnalyzer::operator()(FunctionCall const& _funCall)
);

vector<YulString> argTypes;
for (auto const& arg: _funCall.arguments | boost::adaptors::reversed)
for (size_t i = _funCall.arguments.size(); i > 0; i--)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we create a helper for that? I think the i - 1 is really dangerous. Is there something like python's enumerate? Then we could use for (auto&& [i, arg]: enumerate(_funCall.arguments) | reversed)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes absolutely. I wasn't happy with that either.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@christianparpart do you have a good idea how this could be done?

Copy link
Member

@ekpyron ekpyron Apr 8, 2020

Choose a reason for hiding this comment

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

Maybe just

for (size_t i : boost::counting_range(0, _funCall.arguments.size()) | boost::adaptors::reversed)
{
  auto&& arg = _funCall.arguments[i];

? Or a very simple helper that returns boost::counting_range(0, _container.size())?

I could look into helpers that give you tuples of index and value that are robust against reversing - or even helpers that let you iterate two containers simultanously, so you could use auto&& [needsLiteralArgument, arg]: someWrapper(*needsLiteralArguments, _funCall.arguments) | boost::adaptors::reversed - that's possible, but it can get quite messy quickly :-).

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you see the link I posted above?

Copy link
Member

Choose a reason for hiding this comment

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

By the way: the solution in the link always creates a copy - a more sophisticated solution would use references for lvalues and move for rvalues (again part of the messy-ness ;-))

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine as is. i - 1 is only used twice and the for line is very clear

Copy link
Member

Choose a reason for hiding this comment

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

Maybe change i to something else though? i is also used below, might look confusing visually

Copy link
Contributor

Choose a reason for hiding this comment

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

We have tons of other places in the code where an enumerate or "iterate two lists at the same time" would be very handy.

@chriseth
Copy link
Contributor

chriseth commented Apr 8, 2020

Looks good apart from the enumerate feature. Should we merge this already as is?

@Marenz Marenz force-pushed the immutable-functioncallgraph branch from e93a9af to 5203503 Compare April 8, 2020 14:47
@Marenz Marenz mentioned this pull request Apr 8, 2020
@chriseth chriseth merged commit a7a1feb into develop Apr 9, 2020
@chriseth chriseth deleted the immutable-functioncallgraph branch April 9, 2020 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants