Skip to content

[cling] Replace usage of TextInput with LineEditor #16438

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

devajithvs
Copy link
Contributor

@devajithvs devajithvs commented Sep 16, 2024

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:

  • tested changes locally
  • updated the docs (if necessary)

This PR fixes #

Update tests as errors/warnings will add a new line in TextInput
TextInput does not have a native method to set a history size.
It is handled automatically and set to 800 by default.
We have no way of changing this unless we add a patch on top,
which can potentially be upstreamed.
Add `CLING_HISTSIZE` functionality with the newly added LLVM patch.
@devajithvs devajithvs requested a review from hahnjo September 16, 2024 08:57
@devajithvs devajithvs self-assigned this Sep 16, 2024
@devajithvs devajithvs changed the title [cling] Replace all occurrences of TextInput with LineEditor [cling] Replace usage of TextInput with LineEditor Sep 16, 2024
Copy link

github-actions bot commented Sep 16, 2024

Test Results

    13 files      13 suites   3d 3h 37m 45s ⏱️
 3 032 tests  3 032 ✅ 0 💤 0 ❌
33 895 runs  33 895 ✅ 0 💤 0 ❌

Results for commit 99a1261.

♻️ This comment has been updated with latest results.

@devajithvs devajithvs marked this pull request as ready for review September 25, 2024 08:25
Copy link
Member

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty nice!

Copy link
Member

@hahnjo hahnjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@vgvassilev
Copy link
Member

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.

@devajithvs
Copy link
Contributor Author

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: #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:

::history(Data->Hist, &HE, H_SETSIZE, 800);

@hahnjo
Copy link
Member

hahnjo commented Sep 26, 2024

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.

@devajithvs
Copy link
Contributor Author

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: llvm/llvm-project#110092

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants