root icon indicating copy to clipboard operation
root copied to clipboard

[cling] Delete UITabCompletion* with unique_ptr to prevent memleak

Open jiangyilism opened this issue 3 years ago • 5 comments

Changes or fixes:

A minor memleak in cling::UserInterface::runInteractively() reported by valgrind.

Checklist:

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

jiangyilism avatar Aug 30 '22 08:08 jiangyilism

Can one of the admins verify this patch?

phsft-bot avatar Aug 30 '22 08:08 phsft-bot

@phsft-bot build

Axel-Naumann avatar Aug 30 '22 08:08 Axel-Naumann

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14 How to customize builds

phsft-bot avatar Aug 30 '22 08:08 phsft-bot

Removed the false comment and moved the unique_ptr declaration above TextInputHolder, in case TextInputHolder::~TextInputHolder() still access it (in the future).

However, I am thinking about enhancing CLI completion support of cling. Such as tab completion. Currently cling only prints completion suggestion but does not complete the input buffer for the user when only single unambiguous completion exists. Also the completion does not recognize meta command like .x, .L so it does not try to complete its arguments (paths). And the path does not expand ~ to home dir. @Axel-Naumann , do you think it is a good idea for cling to adopt llvm::LineEditor ? (https://llvm.org/doxygen/classllvm_1_1LineEditor.html) Or building cling's own readline/libedit wrapper?
core/textinput/src/textinput/doc/textinput.txt states that textinput is created to avoid depending on readline/libedit in year 2011. Is it still true today 2022? cling highly depends on llvm now so it should be fine to adopt llvm::LineEditor (created in 2013. textinput predates it). I have a prototype cling with the above enhanced completion support based on libedit but I will only create PR if cling (and all root components currently using textinput ) are open to such changes and dependency. I can refactor to use llvm::LineEditor instead. It is not too hard since the latter is also a libedit wrapper.

jiangyilism avatar Aug 30 '22 20:08 jiangyilism

@Axel-Naumann , @guitargeek Please help approve the github workflow.

jiangyilism avatar Sep 19 '22 13:09 jiangyilism

Thanks for the PR, @jiangyilism - apologies for being so slow!

I'd be very curious to see llvm's libedit work with ROOT and cling. We can certainly use it for cling - that PR would be very welcome!

Axel-Naumann avatar Nov 19 '22 14:11 Axel-Naumann