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

Add support for parsing LuaJIT-specific number suffixes #57

Closed
wants to merge 1 commit into from

Conversation

suchipi
Copy link

@suchipi suchipi commented Jul 20, 2018

This adds support for parsing LuaJIT-specific number suffixes when options.luaVersion is 'LuaJIT'.

See http://luajit.org/ext_ffi_api.html#literals for more info.

Copy link
Owner

@fstirlitz fstirlitz left a comment

Choose a reason for hiding this comment

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

Seems reasonable, some nitpicking aside. Your patch has prompted me to finish up some changes I had , and refactor Lua dialect testing. Unfortunately, this created a couple of conflicts, which I'll leave up to you to resolve.

@@ -889,6 +889,52 @@
};
}

function scanLuaJITImaginaryUnitSuffix() {
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: other scan* function return AST nodes. This one merely a partial token. The prefix should have been read.

}
}

function scanLuaJITInt64Suffix() {
Copy link
Owner

Choose a reason for hiding this comment

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

Likewise.

(foundBinaryExponent && foundInt64Suffix) ||
(foundBinaryExponent && foundImaginaryUnit) ||
(foundImaginaryUnit && foundInt64Suffix)
) {
Copy link
Owner

Choose a reason for hiding this comment

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

Seems a bit convoluted. Why not attempt to scan the suffix and return early?

}
]
},
},
Copy link
Owner

Choose a reason for hiding this comment

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

It seems you've forgotten to commit the corresponding scaffolding file (test/scaffolding/literals).

Copy link
Author

Choose a reason for hiding this comment

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

I wrote this by hand 😅 didn't know about the scaffolding system. I'll fix it

Copy link
Owner

Choose a reason for hiding this comment

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

Well, to be fair, the README should have probably mentioned this...

return true;
} else {
// UL but no L
raise({}, errors.malformedNumber, input.slice(tokenStart, index));
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe the function could return the prefix encountered, if any; this information could be then attached to the final AST node.

Copy link
Owner

Choose a reason for hiding this comment

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

Plus, the triply nested if statements look rather ugly.

@suchipi
Copy link
Author

suchipi commented Feb 4, 2019

I haven't worked on this in ages so I'm gonna close it

@suchipi suchipi closed this Feb 4, 2019
@fstirlitz fstirlitz added the enhancement Request for functionality covering an entirely new use case label Aug 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Request for functionality covering an entirely new use case
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants