Skip to content

Feat: Zero-indexed tables - #3

Closed
lofcz wants to merge 5 commits into
WattleScript:librelancerfrom
lofcz:feat-zero-index-tables
Closed

lofcz wants to merge 5 commits into
WattleScript:librelancerfrom
lofcz:feat-zero-index-tables

Conversation

@lofcz

@lofcz lofcz commented Mar 29, 2022 •

Copy link
Copy Markdown
Member

This PR adds an option on Script config level to index tables from zero. The option itself is implemented as int as it simplifies access (compared to bool impl). I've stated that values other than 1 & 0 are not working but we might as well throw on set if an user attempts to set the value to something else. The setting is set to 1 by default hence this is a nonbreaking change.

Some table operations have been slightly optimized. In particular I've refactored & inlined slow GetIntegralKey method. If-else nesting was reduced in several places and extraneous statements such as else after return in the branch directly above removed.

Opinionated helper property DynValue.Int has been added. I'm open to replace that with a better name. It's just a shorthand for (int)DynValue.Number which we are using all over the place.

A few overloads have been added to the Table. This is mostly to get consistent overloads for various functions. I intend to make a few more of these later.

I've included a new mechanism for loading test cases as the old one requires a line of manual code to be added for every new test. I can refactor old tests to be loaded in this way too.

The structure of a new test is as follows:

mytest.lua <-- lua source code
mytest.txt <-- expected stdout
mytest.cfg <-- might be added later as an optional file to configure lua options

Further commits included:

  • minor cleanup of instructionloop
    • if -> lookup tables where possible
    • removed no longer needed params
    • removed redundant quantifiers
  • added bench project to mock perf ideas against
  • c# bumped from 7.3 to 8.0 (still netstandard 2.0 compatible)
  • CallStackItem refactored to struct
  • avoid zero length array allocations where possible
  • general VM-land code cleanup

@CallumDev

Copy link
Copy Markdown
Contributor

Four things.

  1. This really needs to be split up into multiple PRs. The callstackitem, cleanup, and this zero-indexed table feature can all be separate PRs. This is a -lot- to review and a bit messy.
  2. I would prefer the DynValue integer thing to be something like CastToInteger() since all of the other type name properties are things that a dynvalue can store exactly.
  3. EmbeddedResource is not correct for the .csproj includes for your tests. See https://github.com/Librelancer/moonsharp/blob/librelancer/src/MoonSharp.Tests/MoonSharp.Tests.csproj#L20 for a way of doing a wildcard.
  4. Have a look at commit 97bba70. I've added a method that allows you to do the NUnit asserts directly from the lua code. I think that would be better for your extra tests instead of having extraneous text files, keeps it clean.

@lofcz

lofcz commented Mar 30, 2022

Copy link
Copy Markdown
Member Author

I agree with 1-3, will edit and split the PR accordingly.
As for 4 this is the way tests are done over at Scriban. Your way seems great for cases where more than stdout is needed for asserts but writing lua code out of a string has full support from IDE of choice (linter, syntax highlight, intellisense..)

@lofcz

lofcz commented Mar 30, 2022

Copy link
Copy Markdown
Member Author

Zero-indexed tables extracted to #4

@CallumDev

Copy link
Copy Markdown
Contributor

Done in #4 and #5

@CallumDev CallumDev closed this Mar 31, 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.

2 participants