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

Expose the name of the currently called item #150

Merged
merged 3 commits into from Nov 19, 2022

Conversation

SergioBenitez
Copy link
Contributor

Corollary to #146: so you can retrieve the dynamic name without having to clone it into the filter/function/test.

Copy link
Owner

@mitsuhiko mitsuhiko left a comment

Choose a reason for hiding this comment

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

In a way this also would need to go into FastRecurse and FastSuper since there is a peephole optimization for these basic loop() and super() calls that immediately render.

At the same time those calls infos would only be observable by the engine itself, so paying the extra cost for that seems pointless?

So I think this either should be fixed or a comment should be left in FastRecurse and FastSuper about why they don't update the current call.

}
Instruction::CallMethod(name, arg_count) => {
state.current_call = Some(name);
Copy link
Owner

Choose a reason for hiding this comment

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

This would need to do something like let old_call = mem::replace(&mut state.current_call, Some(name)); since you can have things like {{ foo.bar(blub.blah()) }}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this should just work as-is since foo.bar(blub.blah()) is really:

call blub.blah
pop
call foo.bar

I'll write some tests to confirm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confirmed (no bug) via tests. This change also means you can get rid of the name argument in Object::call_method() if the unwrap() (which should always succeed) is acceptable.

@SergioBenitez
Copy link
Contributor Author

In a way this also would need to go into FastRecurse and FastSuper since there is a peephole optimization for these basic loop() and super() calls that immediately render.

At the same time those calls infos would only be observable by the engine itself, so paying the extra cost for that seems pointless?

So I think this either should be fixed or a comment should be left in FastRecurse and FastSuper about why they don't update the current call.

Indeed, I didn't update the state where it's only visible internally since it provides no value. I'll leave a comment.

@mitsuhiko mitsuhiko merged commit 3b4d992 into mitsuhiko:main Nov 19, 2022
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

2 participants