[RFC] Unify and simplify Iconv implementation (LibC vs LibIconv)

[luislavena]

This is a recap of a side conversation started in #11876:

@asterite commented:

A bit unrelated to this PR, but I think the way the code is right now could be improved. Right now we have this:

# c/iconv.cr

@[Link("iconv")]
lib LibC
  type IconvT = Void*

  fun iconv(x0 : IconvT, x1 : Char**, x2 : SizeT*, x3 : Char**, x4 : SizeT*) : SizeT
  fun iconv_close(x0 : IconvT) : Int
  fun iconv_open(x0 : Char*, x1 : Char*) : IconvT
end

# c/lib_iconv.cr

@[Link("iconv")]
lib LibIconv
  type IconvT = Void*

  alias Int = LibC::Int
  alias Char = LibC::Char
  alias SizeT = LibC::SizeT

  fun iconv = libiconv(cd : IconvT, inbuf : Char**, inbytesleft : SizeT*, outbuf : Char**, outbytesleft : SizeT*) : SizeT
  fun iconv_close = libiconv_close(cd : IconvT) : Int
  fun iconv_open = libiconv_open(tocode : Char*, fromcode : Char*) : IconvT
end

They are exactly the same! Except that:

• One is named LibC and the other is named LibIconv
• One has functions that start with "lib", but their Crystal alias name is the same

So I propose we rename both of these to LibIconv. I think it's actually wrong to put them in LibC if we are linking against "iconv".

Once we do that, we can remove that USE_LIBICONV constant, or maybe not, but all the condition code and macros around that can be gone. That's the whole idea of lib function aliases, so you don't have to do this code switch.

What do you think?

Once we merge this, I think I'll send a PR for this.

Followed by this comment:

The only change is always using LibIconv instead of LibC in some cases and LibIconv in others. Right now the code has a lot of these macros:

{{ USE_LIBICONV ? LibIconv : LibC }}.iconv(@iconv, inbuf, inbytesleft, outbuf, outbytesleft)

With the change I suggest, the become:

LibIconv.iconv(@iconv, inbuf, inbytesleft, outbuf, outbytesleft)

There's absolutely no reason to use two different names here depending on the platform.

And confirmation from @HertzDevil in this comment:

lib_iconv.cr should never be under src/lib_c, because it is not a system library, even if it happens to have the same interface as the system iconv on some (and not all) systems. This is different from LibMPIR masquerading as LibGMP because neither of these are system libraries.

If we simply rename the libs in src/lib_c/*/iconv.cr to LibIconv then that means src/lib_c will no longer contain system library bindings exclusively.

---

I think what @asterite was suggesting is really remove all lib_c/*/c/iconv.cr and get everything unified in lib_iconv.cr. Except for certain platforms (mentioned before), they all share the same Lib interface definition.

Then the only difference if is that is exposed by the libc of that platform or not (Eg musl iconv vs having libiconv explicit linked).

Still not sure the best approach on this (not fully grasped all the needed changes), so decided to move this out of the PR so we can keep track of all the comments.

Thank you! ❤️ ❤️ ❤️

[straight-shoota]

I object to @HertzDevil's comment https://github.com/crystal-lang/crystal/pull/11876#issuecomment-1061873648

If we simply rename the libs in src/lib_c/*/iconv.cr to LibIconv then that means src/lib_c will no longer contain system library bindings exclusively.

Renaming the lib type in these files changes nothing about the target of these bindings. They still refer to the same system library symbols.

Maybe a minor argument could be made about defining system library bindings on a lib type named LibIconv when that's usually LibC. But really, it's just names. Internal names 🤷 The system library bindings for iconv mimic the libiconv API, so I think it's fine to use the name of that library for the lib type.

[HertzDevil]

They still refer to the same system library symbols

Not if use_libiconv is defined, because as I said, libiconv is not a system library to begin with. I believe src/lib_c should not have the possibility of containing non-system bindings even if they are only enabled by a compile-time flag.

The system library bindings for iconv mimic the libiconv API

libiconv mimics the POSIX iconv API, not the other way round.

[straight-shoota]

Not if use_libiconv is defined

The purpose of use_libiconv is not to use the bindings in src/lib_c/*/iconv.cr. The bindings to libiconv wouldn't be in src/lib_c, of course.

This would be the require code for iconv with https://github.com/crystal-lang/crystal/pull/11876 and this issue resolved:

{% if flag?(:use_libiconv) || flag?(:win32) %}
  require "./lib_iconv"
{% else %}
  require "c/iconv"
{% end %}

[HertzDevil]

What would this look like if we reuse the Regex::Engine idiom here?