nealsid added a comment. In D106035#2885292 <https://reviews.llvm.org/D106035#2885292>, @teemperor wrote:
> 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). Definitely +1 on baking it at HEAD for a bit - SGTM. >>> 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. I don't, but maybe I can start that conversation around commit access. For this patch, I'll watch for the next release branch cut and ask you about landing it then. Thanks! Neal 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
