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

exported symbols: avoid wildcard #45

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

anoop142
Copy link
Contributor

@anoop142 anoop142 commented Nov 5, 2023

  • export specific symbols instead of destroy* wildcard
  • order symbols alphabetically

Thanks @agx for suggestions.

* export specific symbols instead of destroy* wildcard
* order symbols alphabetically

Thanks @agx for suggestions.
@agx
Copy link
Contributor

agx commented Nov 5, 2023

Hmmm...somehow github ate my comment:

Thanks a lot. That looks good to me.

I wonder if we shouldn't prefix those with (varnam_) before the next release (#46) to avoid polluting other projects namespaces (we can't do it later on without bumping the so-name). This could happen as a follow up as this MR improves on the current situation a lot already.

One thing I don't grok yet is why those symbols are needed at all as those aren't used in the cli:

go build -o varnamcli -ldflags "-s -w" ./cli
# github.com/varnamproject/govarnam/cli
/usr/lib/go-1.21/pkg/tool/linux_amd64/link: running gcc failed: exit status 1
/usr/bin/ld: /tmp/go-link-3830379946/000002.o: in function `_cgo_583732af04bd_Cfunc_makeSymbol':
/tmp/go-build/govarnamgo.cgo2.c:124:(.text+0x8a): undefined reference to `makeSymbol'
/usr/bin/ld: /tmp/go-link-3830379946/000002.o: in function `_cgo_583732af04bd_Cfunc_destroySchemeDetailsArray':
/tmp/go-build/govarnamgo.cgo2.c:49:(.text+0x4): undefined reference to `destroySchemeDetailsArray'
/usr/bin/ld: /tmp/go-link-3830379946/000002.o: in function `_cgo_583732af04bd_Cfunc_destroySuggestionsArray':
/tmp/go-build/govarnamgo.cgo2.c:61:(.text+0x14): undefined reference to `destroySuggestionsArray'
/usr/bin/ld: /tmp/go-link-3830379946/000002.o: in function `_cgo_583732af04bd_Cfunc_destroySymbolArray':
/tmp/go-build/govarnamgo.cgo2.c:73:(.text+0x24): undefined reference to `destroySymbolArray'
/usr/bin/ld: /tmp/go-link-3830379946/000002.o: in function `_cgo_583732af04bd_Cfunc_destroyTransliterationResult':
/tmp/go-build/govarnamgo.cgo2.c:85:(.text+0x34): undefined reference to `destroyTransliterationResult'

@anoop142
Copy link
Contributor Author

anoop142 commented Nov 5, 2023

prefixing sounds good, even if we are not sure why these symbols are required, prefix for now?

@agx
Copy link
Contributor

agx commented Nov 5, 2023

@anoop142 I'd go with the MR as is for the moment and then look into whether we can't drop those entirely, if not prefix.

@agx
Copy link
Contributor

agx commented Nov 5, 2023

@subins2000 can you say which of these symbols should actually be available to users of the library (e.g. cli or phosh-osk-stub) ?

From what I can tell those are "only" needed for the go-bindings of govarnam (things in govarnamgo/), correct? If so simplest thing would be to prefix them with a different prefix (vgo_)? So we can e.g. exclude them from api doc generation. I don't think it would be worth the effort to have a separate shared object. (note that this is my first interaction with any go-bindings so I'm mostly guessing here)

@subins2000
Copy link
Member

govarnamgo is used by varnamcli: https://github.com/varnamproject/govarnam/tree/master/cli which I intend to package for Debian.

The reason why those destroy* methods are there is because Go handles strings differently, a C char* string needs to be converted to a GoString, when govarnamgo accesses data from the library, the data needs to be converted to a format Go can work on (mostly char* -> GoString conversion). Later the converted data is used in the Go side, so the passed on C data is not needed anymore, hence this data is freed right after the conversion: https://github.com/varnamproject/govarnam/blob/master/govarnamgo/govarnamgo.go#L175

This is a Go specific thing and I don't see it being used in the Java bindings or Ruby bindings (because I think they handle char* directly without trouble). These functions are inside govarnam's exports because it might be useful when bindings are made from other languages. I will prefix them appropriately as said in #46

@agx
Copy link
Contributor

agx commented Nov 20, 2023

Yeah, if you see use with other bindings then can prefixing makes the most sense.

Adding varnamcli to the Debian package is mostly an issue of sorting out this MR and fixing some more build system things (it's one if the motivations for me to look at meson).

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