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

remove const qualifier if required #102

Merged
merged 4 commits into from
Oct 4, 2024

Conversation

axdank
Copy link
Contributor

@axdank axdank commented Oct 2, 2024

fix:

image

@natecraddock
Copy link
Owner

@VisenDev what do you think of these changes?

@VisenDev
Copy link
Contributor

VisenDev commented Oct 4, 2024

I generally oppose using const cast unless completely necessary. However, from what I can tell it does seem to be legal behavior to mutate the result of lua_tolstring.

If mutating the result of a lua_tolstring is defined behavior, then perhaps the correct this to do here is remove the const qualifier from this function

    pub fn toString(lua: *Lua, index: i32) ![:0]const u8 {

Rather than const casting its result.

@natecraddock
Copy link
Owner

What I read on this page https://www.lua.org/manual/5.4/manual.html#lua_tolstring (look for section 4.1.3), it is safe to mutate the result of lua_tolstring but only for a short time (roughly within a function call if I'm reading things right).

I think I'd rather allow the caller to use @constCast() as needed, or provide another function that returns a []u8 with a comment about when it is safe to use.

@natecraddock
Copy link
Owner

natecraddock commented Oct 4, 2024

@axdank do you have any thoughts? Based on the lua reference manual 4.1.3, this could be a possible footgun depending on how the returned string would be used. The safest way would be to duplicate a const string if the string needs to be stored longer than Lua would keep that pointer valid.

I can imagine that someone could use toAny passing a struct with a []u8 in it and then run into issues if they didn't understand that string references can become invalid.

Maybe this is a case for toAnyAlloc? so the allocator can duplicate the const string?

@axdank
Copy link
Contributor Author

axdank commented Oct 4, 2024

@natecraddock How about these changes? I removed the use of @castConst, so if any structure needs a mutable string field, a copy of it is created.

@natecraddock
Copy link
Owner

Thank you for working on this!

@natecraddock natecraddock merged commit baba73e into natecraddock:main Oct 4, 2024
3 checks passed
@axdank axdank deleted the string_const branch October 4, 2024 11:43
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.

3 participants