Skip to content

exported symbols: avoid wildcard#45

Open
anoop142 wants to merge 1 commit into
varnamproject:masterfrom
anoop142:exported-symbols
Open

exported symbols: avoid wildcard#45
anoop142 wants to merge 1 commit into
varnamproject:masterfrom
anoop142:exported-symbols

Conversation

@anoop142
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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