This is an archive of the discontinued LLVM Phabricator instance.

Introduce line editor library.
ClosedPublic

Authored by pcc on Nov 17 2013, 2:56 AM.

Details

Summary

This library will be used by clang-query. I can imagine LLDB becoming another
client of this library, so I think LLVM is a sensible place for it to live.
It wraps libedit, and adds tab completion support.

The code is loosely based on the line editor bits in LLDB, with a few
improvements:

  • Polymorphism for retrieving the list of tab completions, based on the concept pattern from the new pass manager.
  • Tab completion doesn't corrupt terminal output if the input covers multiple lines. Unfortunately this can only be done in a truly horrible way, as far as I can tell. But since the alternative is to implement our own line editor (which I don't think LLVM should be in the business of doing, at least for now) I think it may be acceptable.
  • Includes a fallback for the case where the user doesn't have libedit installed.

Note that this uses C stdio, mainly because libedit also uses C stdio.

Diff Detail

Event Timeline

pcc added a comment.Nov 17 2013, 4:42 PM

I did consider linenoise. My thoughts are that if we were to implement our own line editor, linenoise would be a good starting point. Unfortunately, it does not implement the style of tab completion (show all completions, rather than cycle through each completion) that I think clients such as clang-query would really benefit from, mainly by being able to teach users the matcher syntax as they use it by showing type information for each completing matcher. While libedit does not natively support tab completion, it gives you just enough control to implement it in the way that we need.

Before doing a more in-depth review of the code, i'd want Doug's OK for this to go in in general...

This seems generally useful to me. Looking forward to it growing some functionality beyond a libedit wrapper ;)

pcc added a comment.Jan 23 2014, 6:32 PM

Manuel: ping.

klimek added inline comments.Jan 27 2014, 11:54 AM
lib/LineEditor/LineEditor.cpp
36 ↗(On Diff #5602)

Perhaps "Action"? Completion seems to be clear from the context.

43–53 ↗(On Diff #5602)

I'd pull that out into a method.

55 ↗(On Diff #5602)

That needs an explanation - why is the action kind "ShowCompletions" if we have no common prefix, and "Insert" otherwise?

79 ↗(On Diff #5602)

I'd propose to have an interface, have two .cc files where the libedit one is only compiled if HAVE_LIBEDIT, and allow the main function of a tool to select one and inject it into the edit flow (while the tool that does the edit flow is free to have a default instantiation based on what is available).

pcc updated this revision to Unknown Object (????).Jan 30 2014, 2:24 PM

Address review comments.

lib/LineEditor/LineEditor.cpp
36 ↗(On Diff #5602)

Well, you could have other actions associated with a line editor, like the action when a bound key is pressed or when a line is entered. So I think this name makes things clear.

43–53 ↗(On Diff #5602)

Done.

55 ↗(On Diff #5602)

Added.

79 ↗(On Diff #5602)

So this would allow clients to select the fgets implementation even if libedit is available, right? Is there any use case for such behaviour?

Also, in the long term I think we should have our own line editor implementation here, at which point there will be only one implementation.

klimek accepted this revision.Jan 31 2014, 1:53 AM

lg

lib/LineEditor/LineEditor.cpp
36 ↗(On Diff #5602)

Oh, I didn't mean the class name, I mean the local variable name (basically s/CA/Action/). I think "Action" is in this context easier to understand than CA. I strongly oppose initializations unless they are very very commonly used (an example is SM used in clang; but even there it quickly becomes a problem if it's not consistent, which it is not ;)

55 ↗(On Diff #5602)

Ah, now it all makes sense - thanks!

79 ↗(On Diff #5602)

To me having an interface would be simpler here - I agree that's a trade-off though, and I don't think it needs to block the patch from landing.

pcc closed this revision.Jan 31 2014, 3:52 PM

Closed by commit rL200595 (authored by @pcc).

pcc added inline comments.Jan 31 2014, 3:54 PM
lib/LineEditor/LineEditor.cpp
36 ↗(On Diff #5602)

Makes sense - renamed.