root icon indicating copy to clipboard operation
root copied to clipboard

[cling] Replace usage of TextInput with LineEditor

Open devajithvs opened this issue 1 year ago • 5 comments

This Pull request:

  • Replace all occurrences of TextInput with LLVM's LineEditor library in CLING. This moves the codebase closer to clang-repl and might help in future changes. This will help in removing this whole folder from CLING - https://github.com/root-project/cling/tree/master/lib/UserInterface/textinput, but this doesn't exist in the ROOT repository.
  • Corresponding replacement should also be made in ROOT for complete removal of TextInput, working on it. Will not be as straightforward as this.
  • LineEditor does not support setting history size manually (with CLING_HISTSIZE). For now added this functionality in LLVM; Can later upstream or deprecate this functionality (clang-repl does not have this feature, AFAIK). But ROOT might be relying on this feature - need to look into it. Diff LLVM will fail due to this.
  • A couple of tests were adjusted to make the tests pass.
  • I tried to keep the changes minimal, so we get a proper diff. It can be made more similar to clang-repl later.

Changes or fixes:

Checklist:

  • [x] tested changes locally
  • [ ] updated the docs (if necessary)

This PR fixes #

devajithvs avatar Sep 16 '24 08:09 devajithvs

Test Results

    22 files      22 suites   4d 0h 43m 44s ⏱️  3 707 tests  3 706 ✅ 0 💤 1 ❌ 79 603 runs  79 602 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 1c3c7297.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Sep 16 '24 11:09 github-actions[bot]

We need to decide what to do with CLING_HISTSIZE. If we want to support it, the necessary changes should be upstreamed to LLVM first and then backported to our fork. Note that it would be a first and prevent building against a vanilla LLVM!

This was implemented to be able to set the history to 0 so that we do not get different pointers across gdb runs, iirc.

vgvassilev avatar Sep 25 '24 13:09 vgvassilev

We need to decide what to do with CLING_HISTSIZE. If we want to support it, the necessary changes should be upstreamed to LLVM first and then backported to our fork. Note that it would be a first and prevent building against a vanilla LLVM!

From yesterday's discussion, it seems that this does not align with the idea of using a prebuilt vanilla LLVM. I'm tending towards deprecating CLING_HISTSIZE. The functionality was added here: https://github.com/root-project/root/pull/11286

Regardless, I think the ability to set a history size would be a nice feature for all the tools using it. So, I will try to upstream this anyways. LineEditor hardcodes history size to 800: https://github.com/root-project/root/blob/99a126187f79163b7f5bfc506fa15903f784efbc/interpreter/llvm-project/llvm/lib/LineEditor/LineEditor.cpp#L224

devajithvs avatar Sep 26 '24 07:09 devajithvs

We need to decide what to do with CLING_HISTSIZE. If we want to support it, the necessary changes should be upstreamed to LLVM first and then backported to our fork. Note that it would be a first and prevent building against a vanilla LLVM!

This was implemented to be able to set the history to 0 so that we do not get different pointers across gdb runs, iirc.

Disabling the history can probably be implemented differently, maybe that's enough.

hahnjo avatar Sep 26 '24 07:09 hahnjo

We need to decide what to do with CLING_HISTSIZE. If we want to support it, the necessary changes should be upstreamed to LLVM first and then backported to our fork. Note that it would be a first and prevent building against a vanilla LLVM!

This was implemented to be able to set the history to 0 so that we do not get different pointers across gdb runs, iirc.

Disabling the history can probably be implemented differently, maybe that's enough.

LineEditor doesn't seem to provide a way to do it though. I've opened a PR here: https://github.com/llvm/llvm-project/pull/110092

devajithvs avatar Sep 26 '24 11:09 devajithvs

This is marked draft because standalone cling repository will need textinput directory removed (which can go later but will need a separate commit in the standalone repository)

devajithvs avatar Nov 03 '25 08:11 devajithvs