-
Notifications
You must be signed in to change notification settings - Fork 29
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
Record tag error message includes record name instead of only "userdata" #595
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Vipul! Looks like the PR is not passing the test suite. Can you please look into that?
src/pallene/coder.lua
Outdated
if expected_type == "LUA_TUSERDATA" then | ||
expected_type = C.string(typ.name) | ||
else | ||
expected_type = "pallene_tag_name(" .. expected_type .. ")" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need pallene_tag_name
, now that we have pallene_type_name
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would require both pallene_tag_name
and pallene_type_name
. They are different in two ways.
- Difference in the function signature
const char *pallene_tag_name(int raw_tag)
andconst char *pallene_type_name(lua_State *L, const TValue *v)
expected_type
is known at the compile time (and usespallene_tag_name
) i.e. the argument's expected type. Whereas the argumentconst TValue *v
passed topallene_type_name
cannot be known at compile time.v
is the type of the actual argument passed at runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
IIRC, there aren't any other places that call
pallene_tag_name
. The tag name function was there only for the error messages andpallene_type_name
now does that job. Notice how both functions are currently quite similar. -
If your goal was that the expected_type be known at compile time, then calling
pallene_tag_name
is not the way to do that. That C function call will only happen at run-time. -
One last thing: In the coder.lua, please use different variable names for the type tag returned by
pallene_type_tag
and for the type name that you compute inside the if statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please have a look at the recent changes. Hope the latest commit resolves the issues/suggestions you mentioned.
Hmm, why is our CI failing? 🤔 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it's passing now, go figure...
Thanks again!
Fixes: #151
Code:
Old behaviour
New behaviour