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

Totally untested sscanf utilising code... #240

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Y-Less
Copy link

@Y-Less Y-Less commented Oct 5, 2020

I'm making this PR now as a work tracking PR (because hacktoberfest; if you could add the tag, that would be awesome). I've not tested most of it yet because I can't actually build the plugin (#239), but I know the sscanf lookup and RPC works thanks to other testing (Y-Less/micro). This adds a new function:

native cache_get_native(cache, row, const format[], {Float, _}:...);

This is basically a wrapper around sscanf that passes the row data directly in and allows you to parse it with an sscanf-compatible specifier string. It actually looks for and uses the sscanf plugin (sscanf needs to come first), so there's very little overhead in the code, and if you don't want it just don't include both plugins.

@maddinat0r
Copy link
Collaborator

Thanks for the PR!
I'm not the owner of this repository (sad, I know), so I can't add the hacktoberfest topic. However, if I understood it correctly, adding the "accepted" label should suffice.
I've linked a rough pointer to build the plugin in that issue you created, but if you have more specific questions, feel free to ask.

@Y-Less
Copy link
Author

Y-Less commented Oct 7, 2020

I saw the link, thanks. I'm working on building/testing it now (got past the cmake stage, so making progress). Any comments on the general idea?

@maddinat0r
Copy link
Collaborator

I'm all for it, as long as it's not too intrusive (which it isn't).
One thing I just noticed that I'd change is the log messages. You're currently printing a "failed" log message when the sscanf plugin is not found or has an invalid version, which users might interpret as an error message. I'd remove those completely (fail silently) and only print a log message if the sscanf function has been found successfully.

@Y-Less
Copy link
Author

Y-Less commented Oct 7, 2020

I'll move those to the function itself - not finding the plugin is fine, unless you try calling the function. Actually, the code will not register the native if the sscanf plugin doesn't exist, because you can't call it, but then a user trying to call it without sscanf will get a "function not found" error, despite having the correct version loaded. That's even more confusing, so I could try check which natives they need from the AMX and print a more useful error if it is found in the header.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants