Sorry, I haven't forgotten about this. Haven't had time to sit down for a proper debugging session this or last week. I definitely still plan to update and resend for review. Thanks!

Yikes - issue count starting to slowly increase :) I've reverted the patch, and sorry again about that.

:) Shoot. I definitely want to revert ASAP rather than keep the tree in a bad state. Just curious, since there are timeout messages in the log, would you be able to try rerunning just that one? I see this, too: 0: b"error: '\xe1\x88\xb4' is not a valid command. So I'm guessing the timeout is a red herring.

In D106035#2927660, @teemperor wrote:

The release branch was recently made, so we can land this right now. Just give me a ping when I should merge this :)

Update against HEAD; tested on Debian Buster and Monterey

Update against HEAD (I still need to do a bit more testing but wanted to get the buildbot results in the meantime)

In D106035#2885292, @teemperor wrote:

In D106035#2885174, @nealsid wrote:

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

Reran clang-format and removed extra braces.

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

Not needed with existing test coverage.

Agreed, it looks like there's plenty of test coverage for this scenario, so I'll revert this patch - sorry about that. I'm working on removing the wchar/char preprocessor logic in Editline.cpp so I'll verify the existing coverage before sending that for review. Thanks.

Thanks to both of you for this! I’m traveling for a few days but will be able to take a look towards the end of the week.

Ah, my grep/find skills clearly failed me :)

Add newline.

Awesome. If you or someone could land it for me, that would be appreciated. Thanks!

Sorry, this got a little complicated. On my previous install I had VS 2019 16.5, which ran into the compile error. When I reinstalled, 16.0, 16.6, and 16.9 worked, but 16.4/16.5 did not. Sounds like a super tiny regression for only 2 minor updates. As far as the actual issue, I could not fully understand it, but it seems to have something to do with applying parameter pack arguments while also using default values for std::map template parameters.

Ah, my Windows Boot Camp partition seems to have bit it, so it may be a couple days before I can test & update this, sorry. Please hold off.

Whoops, I did not realize my VS was out of date - appreciate the link. Let me try building with the latest updates, perhaps a later version works.

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

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.

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

Friendly ping - let me know if I misunderstood the comments or if anything else is needed! Thanks again.

In D101250#2718107, @JDevlieghere wrote:

LGTM. I wonder if we would want to wrap this in a macro to get rid of the EditLineConstString duplication while keeping the type safety.

Small update of patch.

Thanks for the review, and apologies for the delay in updating the diffs - seems like I haven't even had an hour to sit down at my dev machine for the past couple weeks, much less actually do any coding :) I don't have commit access, so if you could do that, that would be much appreciated.

Update type in for loop.

Fixed comment.

CR feedback

Thanks for the review - addressed your comments here (I had to rely on Doxygen picking up the type reference automatically because I couldn't get the \see syntax to work correctly, though)

In D98153#2610953, @teemperor wrote:

Thanks for cleaning this up! Are the if (!sc) ... stuff are missing nullptr checks that affect a normal LLDB session (not sure if we can ever have no SymbolContext) or is this just for the unit test?

Anyway, I have some small inline comments but otherwise this looks pretty good to me.

Rebased on HEAD

Fix regression introduced by removing return statement in case where we were able to format a function name.

In D98153#2610953, @teemperor wrote:

Thanks for cleaning this up! Are the if (!sc) ... stuff are missing nullptr checks that affect a normal LLDB session (not sure if we can ever have no SymbolContext) or is this just for the unit test?

Anyway, I have some small inline comments but otherwise this looks pretty good to me.

Thanks - all the comments sound good. I'll upload a new patch.

Reuploading patch after I incorrectly set project repo.

Rebased on HEAD

In D50299#2596141, @JDevlieghere wrote:

In D50299#2596008, @nealsid wrote:

Removed comment on #include line and organized the #includes to match coding guidelines better (I wasn't sure about whether the project uses blank lines in between #includes from different subproject like lldb/clang/llvm so I didn't make the change more broadly)

Looks good. Let me know if you don't have commit access and need me to land this for you.

Removed comment on #include line and organized the #includes to match coding guidelines better (I wasn't sure about whether the project uses blank lines in between #includes from different subproject like lldb/clang/llvm so I didn't make the change more broadly)

Updated diff with run of clang-format and added a check for function pointer validity. Thanks.

In D50299#2589289, @JDevlieghere wrote:

Thanks Neal. Can you run clang-format again and upload the diff with context (git diff -U9999). Overall this looks pretty good.

I'm updating this very old patch that I forgot about in 2018. This one removes the use of batons in the edit line callbacks, replaces typedefs with using declarations, and uses unique_function instead of callbacks.
Thanks,
Neal

Ran clang-format

Fix 80 char line length violations

In D49963#1183036, @clayborg wrote:

This will be very cool. Biggest issues to watch out for is to make sure it doesn't return incorrect values when running for things like the thread count. If you switch to use the "m_debugger.GetCommandInterpreter().GetExecutionContext()" it should solve this by making sure the frame and thread are not filled in when the process is running. It might also be racy. For example if you say "continue", hopefully the process will be resumed by the time the prompt is asked to refresh itself. Since we get events asynchronously this might be a problem.

Addressed code review feedback to move to llvm::unique_function instead of function pointers for callbacks and type aliases instead of typedefs.

Thanks for the comments! Will address and resend for review.

Fix unit test build