[Lldb-commits] [PATCH] D106035: Remove conditional compilation for WCHAR support in libedit

Fri Jul 16 23:15:10 PDT 2021

nealsid added a comment.

In D106035#2879939 <https://reviews.llvm.org/D106035#2879939>, @teemperor wrote:

> I actually expected after the RFC that we would remove all the non-wchar code, but this seems also fine. I think this LGTM in general, but I feel a bit nervous about touching stuff that depends so much on OS/environment. What OS/environment did you test this patch on? Would be nice to have this tested on a few setups before landing.

This was my mistake, sorry.  I originally went the route in this patch and ran into some errors testing, so I switched to what I detailed in the RFC.  But then I found the problem (I was using narrow chars for the GetCharacter callback when that actually isn't supported). Overall I think it is best to use narrow char and string rather than wide-char and wstring because that's more consistent with the rest of LLVM.

Regarding platforms, I tested on OS X Big Sur and Monterey, and Debian Linux inside a VM.  Jan was able to build on Fedora.  I'm happy to test on more platforms - FreeBSD, NetBSD perhaps?

> Also I'm kinda curious if you found any docs/examples that explain whether mixing the wchar/char functions like we do now is actually supported in libedit? IIUC we call now `el_wset` and `el_set` on the same libedit instance. It feels like the `wchar` support in the FreeBSD port was some kind of parallel implementation to the normal `char` support, so I'm surprised that we can just mix those functions and everything still works fine (at least on my Linux machine this seems to work).

Yeah, the original source converts the parameters and calls el_w* functions when the narrow-char functions are called.  This is also true on FreeBSD: https://github.com/freebsd/freebsd-src/blob/373ffc62c158e52cde86a5b934ab4a51307f9f2e/contrib/libedit/eln.c#L359

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106035/new/

https://reviews.llvm.org/D106035