Refactor standard argument verification by Kebap · Pull Request #4661 · Mudlet/Mudlet

Kebap · 2021-01-20T00:30:42Z

Brief overview of PR changes/additions

Refactor standard argument verification to use new getVerifiedX functions instead

Motivation for adding to Mudlet

Remove repetition and increase readability

Other info (issues closed, discussion etc)

Close #4620

add-deployment-links · 2021-01-20T00:30:44Z

Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

You can directly test the changes here:

No need to install anything - just unzip and run.
Let us know if it works well, and if it doesn't, please give details.

SlySven · 2021-01-20T22:39:29Z

src/TLuaInterpreter.cpp

-    if (!lua_isnumber(L, ++s)) {
-        lua_pushfstring(L, "getLines: bad argument #%d type (end line as number expected, got %s!)", s, luaL_typename(L, s));
-        return lua_error(L);
+        windowName = getVerifiedString(L, __func__, 1, "mini console, user window or buffer name {may be omitted for the \"main\" console}", true);


Doesn't the true argument cause some text about this being optional to be included - which is not what the original said exactly - and which won't read quite right with this proposed text?

Original:
getLines: bad argument #%d type (mini console, user window or buffer name as string expected {may be omitted for the \"main\" console}, got %s!)

New:
getLines: bad argument #%d type (mini console, user window or buffer name {may be omitted for the \"main\" console} as string is optional, got %s!)

Yeah I am not sure about all the {comments} you all sprinkled into these sentences in position X or Y:

%s: bad argument #%d type (name as {X} string expected, got %s!)
%s: bad argument #%d type (name as string expected {Y}, got %s!)

For now I just append them to the name and see where we land:

%s: bad argument #%d type (name {Z} as string expected, got %s!)

I say drop the comment in brackets, but may be differing opinions.

Because for the {X} case, some of the functions have different targets - one or two of them also work for labels and some aren't relevant for buffers or the main consoled or mini(sub?)-consoles, that sort of thing...

Actually I find these "may be omitted" comments should stay in the wiki and not the error messages. They are far and few in between, most error messages don't have them at all, and I think it's for a reason.
If you really want to put them in the client somewhere, how about we create a help command, so help(getlines) will print the manual to the player alright?

SlySven · 2021-01-20T22:41:07Z

src/TLuaInterpreter.cpp

    }
-    int lineTo = lua_tointeger(L, s);
+    int lineFrom = getVerifiedInt(L, __func__, s, "start line");
+    s++;


I'd suggest removing this and incorporating it as a pre-increment in the next line...

There were a WHOLE LOT of s++ and ++s shenanigans which I for one found quite irritating, so I try and untangle them where possible. I find explicit and longer sometimes more readable than extreme incorporation. Then again, maybe I sometimes over-simplify. May need to review later.

I have now incorporated all stand-alone s++ and ++s in some other adjacent command.
Would like more feedback if this becomes more readable this way or actually not.

src/TLuaInterpreter.cpp

SlySven · 2021-01-20T23:24:39Z

src/TLuaInterpreter.cpp

-        return lua_error(L);
-    }
-    QString name{lua_tostring(L, 1)};
+    QString name = getVerifiedString(L, __func__, 1, "name");


⚠️ getVerifiedString(...) uses lua_isstring(...) however that will not behave the same as a test for:
(lua_type(L, 1) != LUA_TSTRING)
because, due to Lua's type coercion capabilities lua_isstring(...) will return true if the argument is also a number whereas the other test will not.

Is this a deliberate decision here, or could it be adjusted?
From what I am seeing, currently this openUserWindow function you comment on will throw an error if I try openUserWindow(5) as this is not "real" string it will not let me create that window. If I want a user window with title 5, I need to instead put openUserWindow("5") and that will work fine.
I would like to reason, this is actually a change for the better.
If there are any deliberate choices of LUA_TSTRING as opposed to lua_isstring, please do comment on those. I would love to rewire them to use lua_isstring instead, but if it can't be done I may need to backtrack.

It's good to follow the Lua standard in coercion

It's good to follow the Lua standard in coercion

I did not understand what you are suggesting here.

I assume it means: revert my changes here, then open a new issue to upgrade these functions to the (which?) Lua coercion standard, so maybe SlySven could comply, before this refactoring can happen

If I get @SlySven right he's saying not to do the coercion, I think we should follow Lua here in allowing coercion. That is, do lua_isstring() and instead of (lua_type(L, 1) == LUA_TSTRING)

Yes he (or somebody else?) originally deliberately put LUA_TSTRING and not lua_isstring here.
Now my original change to getVerifiedString would change this 1:1 to use lua_isstring.
That seems not wanted by SlySven, probably because unexpected logic changes.
So the original author or somebody must change logic to use lua_isstring.

I have undone these changes in the 5 affected functions using LUA_T* for argument type checks.

There are 50+ uses of LUA_T* in this file, so probably another undertaking to update them all without breaking logic.

Would you like to file an issue for this updating logic to use coercion? I can't judge if 1:1 is possible without side-effects.

I have undone these changes in the 5 affected functions using LUA_T* for argument type checks.

Then re-done some changes that improve readability, but did not touch coercion this time in those places.

After looking at these 5 functions more, I see no reason why LUA_TSTRING needs to be kept over lua_isstring.
@SlySven please elaborate how they can't be converted in these 5 cases right now.

SlySven

I think you are going to have to rethink some of these refactorings - the ones that originally used a test of the argument type lua_type(lua_state*, int) against LUA_TXXX values only match the particular type whereas the lua_isxxxx(lua_state*, int) tests also allow for coercion from one type to another - particularly strings <==> numbers...

Kebap · 2021-01-20T23:57:39Z

Good comment. Need to review.
There are more than a few dozen different ways we do argument testing throughout TLuaInterpreter.
Need to find some best practice and stick to it as often as possible.

Kebap · 2021-01-21T09:43:54Z

Keep finding and fixing lots of bugs by just looking at these checks over and over again:

vadi2 · 2021-01-21T09:46:39Z

Hm what is the bug? If the argument is invalid, the return quits the for loop and the entire function.

Edit: I see the highlighted and hardcoded 1 now. Hard to spot. Nice.

…m/Mudlet/Mudlet into standardize_argument_verification

vadi2 · 2021-02-02T08:54:26Z

@Mudlet/lua-interface let's review this again 🔨

vadi2

Looks good to me! Would like someone else to review & test before it went in.

This was a massive amount of work that we've needed to do for a while - thanks for doing it!

SlySven

I've taken a couple of hours to go through this and I realise I cannot be certain I've reviewed everything. Just spotted a few things that is all...

SlySven · 2021-02-03T22:43:15Z

src/TLuaInterpreter.cpp

@@ -221,11 +221,11 @@ bool TLuaInterpreter::getVerifiedBool(lua_State* L, const char* functionName, co
 }

 // No documentation available in wiki - internal function
-// See also: verifyBoolean
+// See also: verifyBool


Where is this verifyBool that is listed like this in 5 places?

You'd probably call it bool TLuaInterpreter::getVerifiedBool(lua_State* L, const char* functionName, const int pos, const char* publicName, const bool isOptional) maybe? It was earlier called verifyBoolean. May have missed some renames in comments or such.

SlySven · 2021-02-03T22:54:01Z

src/TLuaInterpreter.cpp

-    }
-    QString uniqueName = lua_tostring(L, 1);
-
+    QString uniqueName = getVerifiedString(L, __func__, 1, "Menu name");
    if (uniqueName == "") {


We do not want unneeded raw string literals - even if they are empty.

Suggested change

if (uniqueName == "") {

if (uniqueName.isEmpty()) {

That line wasn't even touched at all or part of original scope, but I'll give you this one, as it's the only if (x == "") in the whole file...

SlySven · 2021-02-03T22:57:41Z

src/TLuaInterpreter.cpp

-    }
-    int luaFrom = lua_tointeger(L, 2);
-
+    int luaFrom = getVerifiedInt(L, __func__, s, "wrapAt");


luaFrom (and luaTo and luaText in other) places are poorly named generic identifiers that would be better changed to something a bit more meaningful and specific to the function - though - I guess - it would be rightly pointed out that this is not germane to the objectives of this PR.

Indeed it would be, also these are the internal variable names, so to me seem even less important than the names shown to the players which are to be aligned in #4638

SlySven · 2021-02-03T23:51:44Z

src/TLuaInterpreter.cpp

-        return lua_error(L);
-    }
-    int doorStatus = lua_tointeger(L, 3);
+    int doorStatus = getVerifiedInt(L, __func__, 3, "door type  {0=\"none\", 1=\"open\", 2=\"closed\", 3=\"locked\"}");


I thought we were switching to single quotes '?

Thanks! Must have missed this one..

vadi2 · 2021-02-04T07:03:04Z

I think this is ready for PTBs now.

This was introduced by Mudlet#4661. Also revert to using raw string literal empty strings for some lua related functions that do not work with `QString`s directly. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

This was introduced by #4661. Also switch to using raw string literal empty strings for two lua related function calls that do not work with `QString`s directly - which was introduced in #340 by myself! Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

Co-authored-by: Stephen Lyons <slysven@virginmedia.com>

…4768) This was introduced by Mudlet#4661. Also switch to using raw string literal empty strings for two lua related function calls that do not work with `QString`s directly - which was introduced in Mudlet#340 by myself! Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

…...) #### Brief overview of PR changes/additions Revises `(int) TLuaInterpreter::getVerifiedInt(...)` so it checks and errors out should the call of `lua_tointeger(...)` return a value outside the range that can be conveyed as an `int` (actually `int32_t`). This function actaully returns a `lua_Integer` which is a `typedef` of `ptrdiff_t` - which is a signed 32-bit integer on 32-Bit Windows OS but is actually a signed 64-bit integer on all the platforms we currently support. #### Motivation for adding to Mudlet Prevent odd behaviour in the event of integer over/underflows. #### Other info (issues closed, discussion etc) This came about from the changes in 3609e94 as part of Mudlet#4661 in 2021 (which seemes to assume `getVerifiedInt(...)` did actually return an `int`) - ironically I had actually fixed the conversion of 64-bit to 32-bit integers for exit weights in the earlier Mudlet#2106 but which that PR undid. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

……) (#8924) #### Brief overview of PR changes/additions Revises `(int) TLuaInterpreter::getVerifiedInt(...)` so it checks and errors out should the call of `lua_tointeger(...)` return a value outside the range that can be conveyed as an `int` (actually `int32_t`). This function actually returns a `lua_Integer` which is a `typedef` of `ptrdiff_t` - which is a signed 32-bit integer on 32-Bit Windows OS but is actually a signed 64-bit integer on all the platforms we currently support. #### Motivation for adding to Mudlet Prevent odd behaviour in the event of integer over/underflows. #### Other info (issues closed, discussion etc) This came about from the changes in 3609e94 as part of #4661 in 2021 (which seemes to assume `getVerifiedInt(...)` did actually return an `int`) - ironically I had actually fixed the conversion of 64-bit to 32-bit integers for exit weights in the earlier #2106 but which that PR undid. --------- Signed-off-by: Stephen Lyons <slysven@virginmedia.com> Co-authored-by: Zooka <136661366+ZookaOnGit@users.noreply.github.com>

Kebap added 5 commits January 20, 2021 00:13

+1

1e5b07b

+8

64ba6ba

reduce getLines ++s magic

ded4259

+9

55aba08

+27

bdd8bb4

Kebap added 7 commits January 20, 2021 08:26

+10 include untangle setBackgroundColor

24ae846

+4

36fcaca

typo and static cast added

ff270c3

replace static cast by toStdString()

ecd3689

+9

bba7482

more typos

19560ee

undo feedTriggers, keep for later :)

26ec5e3

SlySven reviewed Jan 20, 2021

View reviewed changes

src/TLuaInterpreter.cpp Outdated Show resolved Hide resolved

+23

889380e

SlySven reviewed Jan 20, 2021

View reviewed changes

src/TLuaInterpreter.cpp Outdated Show resolved Hide resolved

SlySven reviewed Jan 20, 2021

View reviewed changes

Kebap added 6 commits January 21, 2021 01:23

+23 + some documentation

12d9274

+10

541b5f3

typos

4da83bc

+23

676aeb4

typos

21fed2b

typo

880812e

+3

dbebbdc

Kebap added 10 commits February 1, 2021 12:55

unchange setWindow

5aa7fcc

unchange createCommandLine

e4f78f0

unchange createCommandLine more

f3845f9

untangle logic in createCommandLine

1341360

untangle logic in createMapper

6bd26e1

Merge branch 'development' into standardize_argument_verification

f6737bd

fix

2b6b8b7

Merge branch 'standardize_argument_verification' of https://github.co…

160c4e6

…m/Mudlet/Mudlet into standardize_argument_verification

fix

3a95394

fix echo()

64c2835

vadi2 self-assigned this Feb 2, 2021

vadi2 approved these changes Feb 3, 2021

View reviewed changes

SlySven reviewed Feb 4, 2021

View reviewed changes

Kebap added 4 commits February 4, 2021 02:42

another single quote

c5a78a9

isEmpty

0e65987

getVerifiedBool

9ead635

Merge branch 'development' into standardize_argument_verification

25b0348

Kebap merged commit 3609e94 into development Feb 4, 2021

Kebap deleted the standardize_argument_verification branch February 4, 2021 08:23

SlySven mentioned this pull request Feb 10, 2021

BugFix: handling of optional 3rd argument to getRoomUserData #4768

Merged

SlySven mentioned this pull request Jul 19, 2021

Fix setBorderSizes() when called with 4 arguments #5325

Merged

Chris7 pushed a commit to Chris7/Mudlet that referenced this pull request Jan 2, 2022

Refactor standard argument verification (Mudlet#4661)

b30583f

Co-authored-by: Stephen Lyons <slysven@virginmedia.com>

SlySven mentioned this pull request Feb 8, 2026

Fix: handle over/underflows in (int) TLuaInterpreter::getVerifiedInt(…) #8924

Merged

Uh oh!

Conversation

Kebap commented Jan 20, 2021

Brief overview of PR changes/additions

Motivation for adding to Mudlet

Other info (issues closed, discussion etc)

Uh oh!

add-deployment-links bot commented Jan 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test versions

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kebap Jan 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SlySven Jan 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SlySven left a comment

Choose a reason for hiding this comment

Uh oh!

Kebap commented Jan 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kebap commented Jan 21, 2021

Uh oh!

vadi2 commented Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vadi2 commented Feb 2, 2021

Uh oh!

vadi2 left a comment

Choose a reason for hiding this comment

Uh oh!

SlySven left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

add-deployment-links bot commented Jan 20, 2021 •

edited

Loading

Kebap Jan 20, 2021 •

edited

Loading

SlySven Jan 20, 2021 •

edited

Loading

Kebap commented Jan 20, 2021 •

edited

Loading

vadi2 commented Jan 21, 2021 •

edited

Loading