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

Add optional argument names to all extern "C" fn types #484

Merged
merged 3 commits into from
Nov 30, 2023

Conversation

webmaster128
Copy link
Member

No description provided.

@webmaster128 webmaster128 force-pushed the vtable-function-arg-names branch from 0a44231 to 1bcc9fd Compare November 29, 2023 18:52
Copy link
Collaborator

@chipshort chipshort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!

gas_meter: *mut gas_meter_t,
gas_used: *mut u64,
key: U8SliceView,
result_out: *mut UnmanagedVector,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Contrary to the comment that was here, this is not really a result. It just contains the value of the kv.Get call, so I would suggest using value_out as name. That name also is more in line with the other db calls.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I'll make the this name more specific. I was more thinking about "return value" here. We have the same case in fn next_key_or_val(. What would be a good name for that? This is the "key or value output, depending on the caller"

Copy link
Collaborator

@chipshort chipshort Nov 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, the naming for that one gets a bit cumbersome. I would either go with "key_or_val_out" to stay in line with the others or just plain "output" which is much simpler.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I just noticed that humanize_address and canonicalize_address also have the same "result" naming.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Will fix all of those to ensure we don't have a different meaning of "result" than the one we are used to from Rust.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Could you double check the last commit? I think the code got a bit cleaner but I am scared I might have messed something up.

Copy link
Collaborator

@chipshort chipshort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Both the added comments and the query_raw change are correct as far as I can tell.

Comment on lines -84 to +85
let result = serde_json::from_slice(&bin_result).or_else(|e| {
Ok(SystemResult::Err(SystemError::InvalidResponse {
let sys_result: SystemResult<_> = serde_json::from_slice(&bin_result).unwrap_or_else(|e| {
SystemResult::Err(SystemError::InvalidResponse {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should behave the same 👍
That type annotation isn't needed, but I'm fine with leaving it in for clarity.

@@ -144,7 +144,7 @@ impl GoIter {
iterator: iterator_t,
gas_meter: *mut gas_meter_t,
gas_limit: *mut u64,
result_out: *mut UnmanagedVector,
key_or_value_out: *mut UnmanagedVector, // key if called from next_key; value if called from next_value
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍

@webmaster128 webmaster128 merged commit 0e597de into main Nov 30, 2023
@webmaster128 webmaster128 deleted the vtable-function-arg-names branch November 30, 2023 15:57
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