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 godocs and free memory in more places #35

Merged
merged 7 commits into from
Jun 18, 2024
Merged

Conversation

gmlewis
Copy link
Contributor

@gmlewis gmlewis commented Jun 17, 2024

This PR is my attempt to more fully document the Go PDK with idiomatic function and variable names along with godoc-style comments.

This is also basically a replacement for PR #34 but I've kept it in two PRs (and left #34 open) in case you want to proceed with #34 separately.

gmlewis added 2 commits June 16, 2024 19:45
Signed-off-by: Glenn Lewis <[email protected]>
extism_pdk.go Show resolved Hide resolved
@zshipko
Copy link
Contributor

zshipko commented Jun 17, 2024

Thanks, I appreciate all the docs!

I don't have strong feelings about the naming of internal functions, does godoc complain? Or is this just easier to read for Go developers? What do you think @nilslice? If everyone is on-board with the renaming changes we can just close #34

As I mentioned in #34 - I have a PR to fix the issue with vars getting freed to early here: extism/js-sdk#71 so we can remove the fix for that from here.

@zshipko zshipko closed this Jun 17, 2024
@zshipko zshipko reopened this Jun 17, 2024
@zshipko
Copy link
Contributor

zshipko commented Jun 17, 2024

I just updated CI, so if you rebase it should run the tests

Signed-off-by: Glenn Lewis <[email protected]>
@nilslice
Copy link
Member

Thanks!

I'm fine renaming the internal functions to be more idiomatic Go names. Since they don't impact the API, it's a pretty harmless change. The main reason I had them in the PDK code named like this was to make them more recognizable when I used them, but it's really no big deal to me.

I think most Go developers would expect them as @gmlewis has them.

@gmlewis
Copy link
Contributor Author

gmlewis commented Jun 17, 2024

Now that I have a better understanding of the memory model thanks to your comments, there are more Memory blocks that should be freed that were created with extismAlloc, like all the extismOutput functions...

I'll work on that later tonight if you agree.

EDIT: I believe I'm done addressing this change.

gmlewis added 2 commits June 17, 2024 16:54
@zshipko
Copy link
Contributor

zshipko commented Jun 17, 2024

Yeah, that sounds good!

And thanks @nilslice, I will close #34

@gmlewis gmlewis changed the title Add godocs and replace #34 Add godocs and free memory in more places Jun 18, 2024
extism_pdk.go Outdated Show resolved Hide resolved
extism_pdk.go Outdated
}

// `GetVarInt` returns the int associated with `key` (or 0 if none).
Copy link
Contributor Author

@gmlewis gmlewis Jun 18, 2024

Choose a reason for hiding this comment

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

One other thing I was very tempted to change here (GetVarInt) was the return signature from int to (int, bool) to be more idiomatic Go (where the bool typically is assigned to a variable called ok and that is used to determine if the key actually exists)... but that would have been a breaking API change, so I left it alone.

If this were changed, then the signature for GetVar should probably also change, but I'm leaving them alone in this PR since I believe it is an orthogonal issue to the memory freeing issue.

Just FYI.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that's a good point!

@zshipko zshipko merged commit de6b7b8 into extism:main Jun 18, 2024
2 checks passed
@gmlewis gmlewis deleted the add-docs branch June 18, 2024 17:40
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