Rename index arg #1459
Closed
Rename index arg #1459
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
cc @dcodeIO |
dcodeIO
reviewed
Sep 6, 2020
Member
dcodeIO
left a comment
dcodeIO
left a comment
There was a problem hiding this comment.
Makes sense to rename it in context of compileCallExpression etc., yeah, except in context of the call_indirect builtin, where it's still an indexArg. Perhaps, a better name for indexArg in call_indirect might be tableIndex.
dcodeIO
reviewed
Sep 6, 2020
dcodeIO
reviewed
Sep 6, 2020
Contributor
Author
|
@dcodeIO does this change still make sense? |
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before we had first class functions, we used this
indexArglocal incompileCallIndirect. From my understanding, this name no longer makes sense because it is not in fact an index into the function table but actually a pointer into a block of memory which contains that index.I picked the name
functionInstancebut I'm not terribly attached to it, I just think thatindexArgis the wrong name (Unless I'm misreading the code of course)This was tripping me up when I was reviewing the code so I figured I'd submit a patch 🤷