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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
