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

Sat Jul 17 03:31:03 PDT 2021

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

In D106035#2885174 <https://reviews.llvm.org/D106035#2885174>, @nealsid wrote:

> 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.

I actually like this approach even better, so I'm glad this turned out the way it did!

> 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?

I think this is more than enough to test on before landing so this LGTM. FWIW, we do have a release branching in about 10 days or so, so you might want to wait with landing this just directly after that branch was made so that this spends a bit more time on ToT where we can find issues before going into a release. (I assume you don't care whether this makes it into the current release or not, but please correct me if i'm wrong).

>> 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

Cool, thanks for finding that out!

Anyway, I think this LGTM. Thanks for doing it! Also I can't recall if you have commit access, so please let me know if I should land this.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106035