Page MenuHomePhabricator

Remove Windows editline from LLDB
ClosedPublic

Authored by nealsid on May 10 2021, 7:25 PM.

Details

Reviewers
kparzysz
stella.stamenova
amccarth
Group Reviewers
Restricted Project
Commits
rG5af3a6645f38: Remove Windows editline from LLDB
Summary

I don't mean to undo others' work but it looks like the hand-rolled EditLine for LLDB on Windows isn't used. It'd be easier to make changes to bring the other platforms' Editline wrapper up to date (e.g. simplifying char vs wchar_t) without modifying/testing this one too.

Diff Detail

Event Timeline

nealsid created this revision.May 10 2021, 7:25 PM
nealsid requested review of this revision.May 10 2021, 7:25 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 10 2021, 7:25 PM
DavidSpickett added inline comments.
llvm/lib/Target/Hexagon/HexagonVectorCombine.cpp
302 ↗(On Diff #344270)

This should be done in a separate change.

teemperor added a subscriber: teemperor.

No idea about how/if this is used, but I think Stella or Adrian are the ones that might be able to answer this.

It looks like this EditLine port was added before I joined this project. @zturner may have worked on it, but I don't know/remember.

If it's actually unused, I have no objection to removing it. I harbor some hope that Windows's steadily improving unix-like terminal implementation (which console programs can enable) will eventually make a Windows-port of EditLine trivial.

I concur that the MSVC version tweak should be a separate change. If you remove that from this patch, I'll LGTM.

nealsid updated this revision to Diff 344489.May 11 2021, 11:01 AM

Updated the diff after moving an unrelated change into another revision.

nealsid edited the summary of this revision. (Show Details)May 11 2021, 11:03 AM

I don't have context into this either. Does this cause any test failures or is everything still green after the change?

I ran LLDB tests from a clean tree and with my change and the failures seemed to be the same, but there is a large amount of them. I also did manual testing with the changes. Build is still green.

Editline is only used when Cmake can find the editline libraries, which it never does on Windows. So the code that is actually used for console I/O is the code in the IOHandlerEditline class in IOHandler.cpp that is outside of the #ifdef LLDB_USE_EDITLINE preprocessor check. That uses POSIX functions for console I/O. I think it may be a good follow up change to make IOHandlerEditline be used in editline-specific cases and move the fallback/non-editline code in that class to another class.

amccarth accepted this revision.May 11 2021, 3:02 PM

LGTM. Dead code should go. If somebody wants to own a Windows port of EditLine, they can re-instate it and put some tests on it.

Thanks.

This revision is now accepted and ready to land.May 11 2021, 3:02 PM

Thanks, all. If there are no more comments, could someone land it for me? I don't have commit access.

This revision was landed with ongoing or failed builds.May 12 2021, 2:05 AM
This revision was automatically updated to reflect the committed changes.

Thanks, all. If there are no more comments, could someone land it for me? I don't have commit access.

Done.