[Lldb-commits] [PATCH] D106035: Remove conditional compilation for WCHAR support in libedit

[Neal via Phabricator via lldb-commits]

nealsid added a comment.

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!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106035/new/

https://reviews.llvm.org/D106035

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106035: Remove conditional compilation for WCHAR support in libedit

[Neal via Phabricator via lldb-commits]

nealsid added a comment.

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

I have to travel for a couple days but when I get my dev machine back up later 
this week I will investigate these issues in detail.  I focussed my testing 
more on Unicode identifiers inside source files/debug information and missed 
the backspace problem, unfortunately.  Thanks.

In D106035#2927942 , @teemperor wrote:

> I ran it a few times and I'm sure it's an actual '"the output didn't show up" 
> timeout and not a random failure. I also checked and my node doesn't have an 
> `.editrc` file or something like that on the node. It's failing on a 
> different Arch Linux bot so I think it's not a borked machine.
>
> Arch should use just the vanilla libedit so I am not sure why it would be 
> different from other distros. IIRC I ran all the tests when I first reviewed 
> this, so I am also a bit confused why this is now failing. When I manually 
> input unicode characters into the old and new prompt both seem to work, so 
> maybe it's the pexpect test setup that is somehow dropping the input?
>
> On a side note: I think this patch actually breaks the way backspace works. 
> With this patch applied, if I enter a command such as '∂' and then 
> press backspace, the cursor is now moved to the right by (what I assume) is 
> the amount of bytes that I entered. I think there is some strlen logic going 
> on that isn't unicode aware. See below: F18365237: Screenshot 2021-08-05 at 
> 11.51.41.png 
>
> In D106035#2927847 , @nealsid wrote:
>
>> :) 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.
>>
>> I'll create an Arch VM to test on for future patches.  Thank you and sorry 
>> about that.
>>
>> In D106035#2927817 , @teemperor 
>> wrote:
>>
>>> I fear I might also have to congratulate you on your first revert (sorry, 
>>> couldn't resist that joke). TestUnicode is failing for me with this patch 
>>> on Arch Linux:
>>>
>>>   Command Output (stdout):
>>>   --
>>>   lldb version 14.0.0 (g...@github.com:Teemperor/llvm-project revision 
>>> f9f63e92243ecd6d0df5d6813967a49c8ca79ca5)
>>> clang revision f9f63e92243ecd6d0df5d6813967a49c8ca79ca5
>>> llvm revision f9f63e92243ecd6d0df5d6813967a49c8ca79ca5
>>>   Skipping the following test categories: ['libc++', 'dsym', 'gmodules', 
>>> 'debugserver', 'objc']
>>>   
>>>   --
>>>   Command Output (stderr):
>>>   --
>>>   FAIL: LLDB (/home/teemperor/work/ci/build/bin/clang-x86_64) :: 
>>> test_unicode_input (TestUnicode.TestCase)
>>>   ==
>>>   ERROR: test_unicode_input (TestUnicode.TestCase)
>>>   --
>>>   Traceback (most recent call last):
>>> File 
>>> "/home/teemperor/work/ci/llvm-project/lldb/third_party/Python/module/pexpect-4.6/pexpect/expect.py",
>>>  line 111, in expect_loop
>>>   incoming = spawn.read_nonblocking(spawn.maxread, timeout)
>>> File 
>>> "/home/teemperor/work/ci/llvm-project/lldb/third_party/Python/module/pexpect-4.6/pexpect/pty_spawn.py",
>>>  line 482, in read_nonblocking
>>>   raise TIMEOUT('Timeout exceeded.')
>>>   pexpect.exceptions.TIMEOUT: Timeout exceeded.
>>>   
>>>   During handling of the above exception, another exception occurred:
>>>   
>>>   Traceback (most recent call last):
>>> File 
>>> "/home/teemperor/work/ci/llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py",
>>>  line 149, in wrapper
>>>   return func(*args, **kwargs)
>>> File 
>>> "/home/teemperor/work/ci/llvm-project/lldb/test/API/iohandler/unicode/TestUnicode.py",
>>>  line 25, in test_unicode_input
>>>   self.expect(u'\u1234', substrs=[u"error: '\u1234' is not a valid 
>>> command.".encode('utf-8')])
>>> File 
>>> "/home/teemperor/work/ci/llvm-project/lldb/packages/Python/lldbsuite/test/lldbpexpect.py",
>>>  line 64, in expect
>>>   self.child.expect_exact(s)
>>> File 
>>> "/home/teemperor/work/ci/llvm-project/lldb/third_party/Python/module/pexpect-4.6/pexpect/spawnbase.py",
>>>  line 418, in expect_exact
>>>   return exp.expect_loop(timeout)
>>> File 
>>> "/home/teemperor/work/ci/llvm-project/lldb/third_party/Python/module/pexpect-4.6/pexpect/expect.py",
>>>  line 119, in expect_loop
>>>   return self.timeout(e)
>>> File 
>>> "/home/teemperor/work/ci/llvm-project/lldb/third_party/Python/module/pexpect-4.6/pexpect/expect.py",
>>>  line 82, in timeout
>>>   raise TIMEOUT(msg)
>>>   pexpect.exceptions

[Lldb-commits] [PATCH] D106035: Remove conditional compilation for WCHAR support in libedit

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

I ran it a few times and I'm sure it's an actual '"the output didn't show up" 
timeout and not a random failure. I also checked and my node doesn't have an 
`.editrc` file or something like that on the node. It's failing on a different 
Arch Linux bot so I think it's not a borked machine.

Arch should use just the vanilla libedit so I am not sure why it would be 
different from other distros. IIRC I ran all the tests when I first reviewed 
this, so I am also a bit confused why this is now failing. When I manually 
input unicode characters into the old and new prompt both seem to work, so 
maybe it's the pexpect test setup that is somehow dropping the input?

On a side note: I think this patch actually breaks the way backspace works. 
With this patch applied, if I enter a command such as '∂' and then 
press backspace, the cursor is now moved to the right by (what I assume) is the 
amount of bytes that I entered. I think there is some strlen logic going on 
that isn't unicode aware. See below: F18365237: Screenshot 2021-08-05 at 
11.51.41.png 

In D106035#2927847 , @nealsid wrote:

> :) 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.
>
> I'll create an Arch VM to test on for future patches.  Thank you and sorry 
> about that.
>
> In D106035#2927817 , @teemperor 
> wrote:
>
>> I fear I might also have to congratulate you on your first revert (sorry, 
>> couldn't resist that joke). TestUnicode is failing for me with this patch on 
>> Arch Linux:
>>
>>   Command Output (stdout):
>>   --
>>   lldb version 14.0.0 (g...@github.com:Teemperor/llvm-project revision 
>> f9f63e92243ecd6d0df5d6813967a49c8ca79ca5)
>> clang revision f9f63e92243ecd6d0df5d6813967a49c8ca79ca5
>> llvm revision f9f63e92243ecd6d0df5d6813967a49c8ca79ca5
>>   Skipping the following test categories: ['libc++', 'dsym', 'gmodules', 
>> 'debugserver', 'objc']
>>   
>>   --
>>   Command Output (stderr):
>>   --
>>   FAIL: LLDB (/home/teemperor/work/ci/build/bin/clang-x86_64) :: 
>> test_unicode_input (TestUnicode.TestCase)
>>   ==
>>   ERROR: test_unicode_input (TestUnicode.TestCase)
>>   --
>>   Traceback (most recent call last):
>> File 
>> "/home/teemperor/work/ci/llvm-project/lldb/third_party/Python/module/pexpect-4.6/pexpect/expect.py",
>>  line 111, in expect_loop
>>   incoming = spawn.read_nonblocking(spawn.maxread, timeout)
>> File 
>> "/home/teemperor/work/ci/llvm-project/lldb/third_party/Python/module/pexpect-4.6/pexpect/pty_spawn.py",
>>  line 482, in read_nonblocking
>>   raise TIMEOUT('Timeout exceeded.')
>>   pexpect.exceptions.TIMEOUT: Timeout exceeded.
>>   
>>   During handling of the above exception, another exception occurred:
>>   
>>   Traceback (most recent call last):
>> File 
>> "/home/teemperor/work/ci/llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py",
>>  line 149, in wrapper
>>   return func(*args, **kwargs)
>> File 
>> "/home/teemperor/work/ci/llvm-project/lldb/test/API/iohandler/unicode/TestUnicode.py",
>>  line 25, in test_unicode_input
>>   self.expect(u'\u1234', substrs=[u"error: '\u1234' is not a valid 
>> command.".encode('utf-8')])
>> File 
>> "/home/teemperor/work/ci/llvm-project/lldb/packages/Python/lldbsuite/test/lldbpexpect.py",
>>  line 64, in expect
>>   self.child.expect_exact(s)
>> File 
>> "/home/teemperor/work/ci/llvm-project/lldb/third_party/Python/module/pexpect-4.6/pexpect/spawnbase.py",
>>  line 418, in expect_exact
>>   return exp.expect_loop(timeout)
>> File 
>> "/home/teemperor/work/ci/llvm-project/lldb/third_party/Python/module/pexpect-4.6/pexpect/expect.py",
>>  line 119, in expect_loop
>>   return self.timeout(e)
>> File 
>> "/home/teemperor/work/ci/llvm-project/lldb/third_party/Python/module/pexpect-4.6/pexpect/expect.py",
>>  line 82, in timeout
>>   raise TIMEOUT(msg)
>>   pexpect.exceptions.TIMEOUT: Timeout exceeded.
>>   
>>   command: /home/teemperor/work/ci/build/bin/lldb
>>   args: ['/home/teemperor/work/ci/build/bin/lldb', '--no-lldbinit', 
>> '--no-use-colors', '-O', 'settings clear -all', '-O', 'settings set 
>> symbols.enable-external-lookup false', '-O', 'settings set 
>> target.inherit-tcc true', '-O', 'settings set target.detach-on-error false', 
>> '-O', 'settings set target.auto-apply-fixits false', '-O', 'settings set 
>> plugin.process.gdb-remote.packet-timeout 60', '-O', 'settings set 
>> symbols.clang-modules-cache-path 
>> "/home/teemperor

[Lldb-commits] [PATCH] D106035: Remove conditional compilation for WCHAR support in libedit

[Neal via Phabricator via lldb-commits]

nealsid added a comment.

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

I'll create an Arch VM to test on for future patches.  Thank you and sorry 
about that.

In D106035#2927817 , @teemperor wrote:

> I fear I might also have to congratulate you on your first revert (sorry, 
> couldn't resist that joke). TestUnicode is failing for me with this patch on 
> Arch Linux:
>
>   Command Output (stdout):
>   --
>   lldb version 14.0.0 (g...@github.com:Teemperor/llvm-project revision 
> f9f63e92243ecd6d0df5d6813967a49c8ca79ca5)
> clang revision f9f63e92243ecd6d0df5d6813967a49c8ca79ca5
> llvm revision f9f63e92243ecd6d0df5d6813967a49c8ca79ca5
>   Skipping the following test categories: ['libc++', 'dsym', 'gmodules', 
> 'debugserver', 'objc']
>   
>   --
>   Command Output (stderr):
>   --
>   FAIL: LLDB (/home/teemperor/work/ci/build/bin/clang-x86_64) :: 
> test_unicode_input (TestUnicode.TestCase)
>   ==
>   ERROR: test_unicode_input (TestUnicode.TestCase)
>   --
>   Traceback (most recent call last):
> File 
> "/home/teemperor/work/ci/llvm-project/lldb/third_party/Python/module/pexpect-4.6/pexpect/expect.py",
>  line 111, in expect_loop
>   incoming = spawn.read_nonblocking(spawn.maxread, timeout)
> File 
> "/home/teemperor/work/ci/llvm-project/lldb/third_party/Python/module/pexpect-4.6/pexpect/pty_spawn.py",
>  line 482, in read_nonblocking
>   raise TIMEOUT('Timeout exceeded.')
>   pexpect.exceptions.TIMEOUT: Timeout exceeded.
>   
>   During handling of the above exception, another exception occurred:
>   
>   Traceback (most recent call last):
> File 
> "/home/teemperor/work/ci/llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py",
>  line 149, in wrapper
>   return func(*args, **kwargs)
> File 
> "/home/teemperor/work/ci/llvm-project/lldb/test/API/iohandler/unicode/TestUnicode.py",
>  line 25, in test_unicode_input
>   self.expect(u'\u1234', substrs=[u"error: '\u1234' is not a valid 
> command.".encode('utf-8')])
> File 
> "/home/teemperor/work/ci/llvm-project/lldb/packages/Python/lldbsuite/test/lldbpexpect.py",
>  line 64, in expect
>   self.child.expect_exact(s)
> File 
> "/home/teemperor/work/ci/llvm-project/lldb/third_party/Python/module/pexpect-4.6/pexpect/spawnbase.py",
>  line 418, in expect_exact
>   return exp.expect_loop(timeout)
> File 
> "/home/teemperor/work/ci/llvm-project/lldb/third_party/Python/module/pexpect-4.6/pexpect/expect.py",
>  line 119, in expect_loop
>   return self.timeout(e)
> File 
> "/home/teemperor/work/ci/llvm-project/lldb/third_party/Python/module/pexpect-4.6/pexpect/expect.py",
>  line 82, in timeout
>   raise TIMEOUT(msg)
>   pexpect.exceptions.TIMEOUT: Timeout exceeded.
>   
>   command: /home/teemperor/work/ci/build/bin/lldb
>   args: ['/home/teemperor/work/ci/build/bin/lldb', '--no-lldbinit', 
> '--no-use-colors', '-O', 'settings clear -all', '-O', 'settings set 
> symbols.enable-external-lookup false', '-O', 'settings set target.inherit-tcc 
> true', '-O', 'settings set target.detach-on-error false', '-O', 'settings set 
> target.auto-apply-fixits false', '-O', 'settings set 
> plugin.process.gdb-remote.packet-timeout 60', '-O', 'settings set 
> symbols.clang-modules-cache-path 
> "/home/teemperor/work/ci/build/lldb-test-build.noindex/module-cache-lldb/lldb-api"',
>  '-O', 'settings set use-color false', '-O', 'setting set 
> target.prefer-dynamic-value no-dynamic-values']
>   buffer (last 100 chars): b'\\U+1234\r\n(lldb) '
>   before (last 100 chars): b'\\U+1234\r\n(lldb) '
>   after: 
>   match: None
>   match_index: None
>   exitstatus: None
>   flag_eof: False
>   pid: 609226
>   child_fd: 6
>   closed: False
>   timeout: 60
>   delimiter: 
>   logfile: None
>   logfile_read: None
>   logfile_send: None
>   maxread: 2000
>   ignorecase: False
>   searchwindowsize: None
>   delaybeforesend: 0.05
>   delayafterclose: 0.1
>   delayafterterminate: 0.1
>   searcher: searcher_string:
>   0: b"error: '\xe1\x88\xb4' is not a valid command."
>   Config=x86_64-/home/teemperor/work/ci/build/bin/clang
>   --
>   Ran 1 test in 60.551s




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106035/new/

https://reviews.llvm.org/D106035

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106035: Remove conditional compilation for WCHAR support in libedit

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

I fear I might also have to congratulate you on your first revert (sorry, 
couldn't resist that joke). TestUnicode is failing for me with this patch on 
Arch Linux:

  Command Output (stdout):
  --
  lldb version 14.0.0 (g...@github.com:Teemperor/llvm-project revision 
f9f63e92243ecd6d0df5d6813967a49c8ca79ca5)
clang revision f9f63e92243ecd6d0df5d6813967a49c8ca79ca5
llvm revision f9f63e92243ecd6d0df5d6813967a49c8ca79ca5
  Skipping the following test categories: ['libc++', 'dsym', 'gmodules', 
'debugserver', 'objc']
  
  --
  Command Output (stderr):
  --
  FAIL: LLDB (/home/teemperor/work/ci/build/bin/clang-x86_64) :: 
test_unicode_input (TestUnicode.TestCase)
  ==
  ERROR: test_unicode_input (TestUnicode.TestCase)
  --
  Traceback (most recent call last):
File 
"/home/teemperor/work/ci/llvm-project/lldb/third_party/Python/module/pexpect-4.6/pexpect/expect.py",
 line 111, in expect_loop
  incoming = spawn.read_nonblocking(spawn.maxread, timeout)
File 
"/home/teemperor/work/ci/llvm-project/lldb/third_party/Python/module/pexpect-4.6/pexpect/pty_spawn.py",
 line 482, in read_nonblocking
  raise TIMEOUT('Timeout exceeded.')
  pexpect.exceptions.TIMEOUT: Timeout exceeded.
  
  During handling of the above exception, another exception occurred:
  
  Traceback (most recent call last):
File 
"/home/teemperor/work/ci/llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py",
 line 149, in wrapper
  return func(*args, **kwargs)
File 
"/home/teemperor/work/ci/llvm-project/lldb/test/API/iohandler/unicode/TestUnicode.py",
 line 25, in test_unicode_input
  self.expect(u'\u1234', substrs=[u"error: '\u1234' is not a valid 
command.".encode('utf-8')])
File 
"/home/teemperor/work/ci/llvm-project/lldb/packages/Python/lldbsuite/test/lldbpexpect.py",
 line 64, in expect
  self.child.expect_exact(s)
File 
"/home/teemperor/work/ci/llvm-project/lldb/third_party/Python/module/pexpect-4.6/pexpect/spawnbase.py",
 line 418, in expect_exact
  return exp.expect_loop(timeout)
File 
"/home/teemperor/work/ci/llvm-project/lldb/third_party/Python/module/pexpect-4.6/pexpect/expect.py",
 line 119, in expect_loop
  return self.timeout(e)
File 
"/home/teemperor/work/ci/llvm-project/lldb/third_party/Python/module/pexpect-4.6/pexpect/expect.py",
 line 82, in timeout
  raise TIMEOUT(msg)
  pexpect.exceptions.TIMEOUT: Timeout exceeded.
  
  command: /home/teemperor/work/ci/build/bin/lldb
  args: ['/home/teemperor/work/ci/build/bin/lldb', '--no-lldbinit', 
'--no-use-colors', '-O', 'settings clear -all', '-O', 'settings set 
symbols.enable-external-lookup false', '-O', 'settings set target.inherit-tcc 
true', '-O', 'settings set target.detach-on-error false', '-O', 'settings set 
target.auto-apply-fixits false', '-O', 'settings set 
plugin.process.gdb-remote.packet-timeout 60', '-O', 'settings set 
symbols.clang-modules-cache-path 
"/home/teemperor/work/ci/build/lldb-test-build.noindex/module-cache-lldb/lldb-api"',
 '-O', 'settings set use-color false', '-O', 'setting set 
target.prefer-dynamic-value no-dynamic-values']
  buffer (last 100 chars): b'\\U+1234\r\n(lldb) '
  before (last 100 chars): b'\\U+1234\r\n(lldb) '
  after: 
  match: None
  match_index: None
  exitstatus: None
  flag_eof: False
  pid: 609226
  child_fd: 6
  closed: False
  timeout: 60
  delimiter: 
  logfile: None
  logfile_read: None
  logfile_send: None
  maxread: 2000
  ignorecase: False
  searchwindowsize: None
  delaybeforesend: 0.05
  delayafterclose: 0.1
  delayafterterminate: 0.1
  searcher: searcher_string:
  0: b"error: '\xe1\x88\xb4' is not a valid command."
  Config=x86_64-/home/teemperor/work/ci/build/bin/clang
  --
  Ran 1 test in 60.551s


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106035/new/

https://reviews.llvm.org/D106035

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106035: Remove conditional compilation for WCHAR support in libedit

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

In D106035#2927714 , @nealsid wrote:

> 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 :)
>
> Thanks - I recently got commit access so I figured I'd try it out with this 
> one, and I *think* it worked.

Jepp, worked and congrats!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106035/new/

https://reviews.llvm.org/D106035

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106035: Remove conditional compilation for WCHAR support in libedit

[Neal via Phabricator via lldb-commits]

nealsid added a comment.

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

Thanks - I recently got commit access so I figured I'd try it out with this 
one, and I *think* it worked.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106035/new/

https://reviews.llvm.org/D106035

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106035: Remove conditional compilation for WCHAR support in libedit

[Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG7529f0e3e142: D106035: Remove conditional compilation for 
WCHAR support in libedit (authored by Neal Sidhwaney 
).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106035/new/

https://reviews.llvm.org/D106035

Files:
  lldb/cmake/modules/LLDBConfig.cmake
  lldb/include/lldb/Host/Editline.h
  lldb/source/Host/common/Editline.cpp

Index: lldb/source/Host/common/Editline.cpp
===
--- lldb/source/Host/common/Editline.cpp
+++ lldb/source/Host/common/Editline.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include 
+#include 
 #include 
 
 #include "lldb/Host/Editline.h"
@@ -61,40 +62,12 @@
 #define ANSI_UP_N_ROWS ESCAPE "[%dA"
 #define ANSI_DOWN_N_ROWS ESCAPE "[%dB"
 
-#if LLDB_EDITLINE_USE_WCHAR
-
-#define EditLineConstString(str) L##str
-#define EditLineStringFormatSpec "%ls"
-
-#else
-
 #define EditLineConstString(str) str
 #define EditLineStringFormatSpec "%s"
 
-// use #defines so wide version functions and structs will resolve to old
-// versions for case of libedit not built with wide char support
-#define history_w history
-#define history_winit history_init
-#define history_wend history_end
-#define HistoryW History
-#define HistEventW HistEvent
-#define LineInfoW LineInfo
-
-#define el_wgets el_gets
-#define el_wgetc el_getc
-#define el_wpush el_push
-#define el_wparse el_parse
-#define el_wset el_set
-#define el_wget el_get
-#define el_wline el_line
-#define el_winsertstr el_insertstr
-#define el_wdeletestr el_deletestr
-
-#endif // #if LLDB_EDITLINE_USE_WCHAR
-
-bool IsOnlySpaces(const EditLineStringType &content) {
-  for (wchar_t ch : content) {
-if (ch != EditLineCharType(' '))
+bool IsOnlySpaces(const std::string &content) {
+  for (char ch : content) {
+if (ch != ' ')
   return false;
   }
   return true;
@@ -132,17 +105,16 @@
   llvm_unreachable("Fully covered switch!");
 }
 
-
-EditLineStringType CombineLines(const std::vector &lines) {
-  EditLineStringStreamType combined_stream;
-  for (EditLineStringType line : lines) {
+std::string CombineLines(const std::vector &lines) {
+  std::stringstream combined_stream;
+  for (const std::string &line : lines) {
 combined_stream << line.c_str() << "\n";
   }
   return combined_stream.str();
 }
 
-std::vector SplitLines(const EditLineStringType &input) {
-  std::vector result;
+std::vector SplitLines(const std::string &input) {
+  std::vector result;
   size_t start = 0;
   while (start < input.length()) {
 size_t end = input.find('\n', start);
@@ -161,23 +133,18 @@
   return result;
 }
 
-EditLineStringType FixIndentation(const EditLineStringType &line,
-  int indent_correction) {
+std::string FixIndentation(const std::string &line, int indent_correction) {
   if (indent_correction == 0)
 return line;
   if (indent_correction < 0)
 return line.substr(-indent_correction);
-  return EditLineStringType(indent_correction, EditLineCharType(' ')) + line;
+  return std::string(indent_correction, ' ') + line;
 }
 
-int GetIndentation(const EditLineStringType &line) {
-  int space_count = 0;
-  for (EditLineCharType ch : line) {
-if (ch != EditLineCharType(' '))
-  break;
-++space_count;
-  }
-  return space_count;
+int GetIndentation(const std::string &line) {
+  auto firstNonSpace = std::find_if(line.begin(), line.end(),
+[](const char ch) { return ch != ' '; });
+  return firstNonSpace - line.begin();
 }
 
 bool IsInputPending(FILE *file) {
@@ -206,10 +173,10 @@
   // these objects
   EditlineHistory(const std::string &prefix, uint32_t size, bool unique_entries)
   : m_history(nullptr), m_event(), m_prefix(prefix), m_path() {
-m_history = history_winit();
-history_w(m_history, &m_event, H_SETSIZE, size);
+m_history = history_init();
+history(m_history, &m_event, H_SETSIZE, size);
 if (unique_entries)
-  history_w(m_history, &m_event, H_SETUNIQUE, 1);
+  history(m_history, &m_event, H_SETUNIQUE, 1);
   }
 
   const char *GetHistoryFilePath() {
@@ -222,11 +189,7 @@
   // LLDB stores its history in ~/.lldb/. If for some reason this directory
   // isn't writable or cannot be created, history won't be available.
   if (!llvm::sys::fs::create_directory(lldb_history_file)) {
-#if LLDB_EDITLINE_USE_WCHAR
-std::string filename = m_prefix + "-widehistory";
-#else
 std::string filename = m_prefix + "-history";
-#endif
 llvm::sys::path::append(lldb_history_file, filename);
 m_path = std::string(lldb_history_file.str());
   }
@@ -243,7 +206,7 @@
 Save();
 
 if (m_history) {
-  history_wend(m_history);
+  history_end(m_hist

[Lldb-commits] [PATCH] D106035: Remove conditional compilation for WCHAR support in libedit

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106035/new/

https://reviews.llvm.org/D106035

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106035: Remove conditional compilation for WCHAR support in libedit

[Neal via Phabricator via lldb-commits]

nealsid updated this revision to Diff 364362.
nealsid added a comment.

Update against HEAD; tested on Debian Buster and Monterey


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106035/new/

https://reviews.llvm.org/D106035

Files:
  lldb/cmake/modules/LLDBConfig.cmake
  lldb/include/lldb/Host/Editline.h
  lldb/source/Host/common/Editline.cpp

Index: lldb/source/Host/common/Editline.cpp
===
--- lldb/source/Host/common/Editline.cpp
+++ lldb/source/Host/common/Editline.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include 
+#include 
 #include 
 
 #include "lldb/Host/Editline.h"
@@ -61,40 +62,12 @@
 #define ANSI_UP_N_ROWS ESCAPE "[%dA"
 #define ANSI_DOWN_N_ROWS ESCAPE "[%dB"
 
-#if LLDB_EDITLINE_USE_WCHAR
-
-#define EditLineConstString(str) L##str
-#define EditLineStringFormatSpec "%ls"
-
-#else
-
 #define EditLineConstString(str) str
 #define EditLineStringFormatSpec "%s"
 
-// use #defines so wide version functions and structs will resolve to old
-// versions for case of libedit not built with wide char support
-#define history_w history
-#define history_winit history_init
-#define history_wend history_end
-#define HistoryW History
-#define HistEventW HistEvent
-#define LineInfoW LineInfo
-
-#define el_wgets el_gets
-#define el_wgetc el_getc
-#define el_wpush el_push
-#define el_wparse el_parse
-#define el_wset el_set
-#define el_wget el_get
-#define el_wline el_line
-#define el_winsertstr el_insertstr
-#define el_wdeletestr el_deletestr
-
-#endif // #if LLDB_EDITLINE_USE_WCHAR
-
-bool IsOnlySpaces(const EditLineStringType &content) {
-  for (wchar_t ch : content) {
-if (ch != EditLineCharType(' '))
+bool IsOnlySpaces(const std::string &content) {
+  for (char ch : content) {
+if (ch != ' ')
   return false;
   }
   return true;
@@ -132,17 +105,16 @@
   llvm_unreachable("Fully covered switch!");
 }
 
-
-EditLineStringType CombineLines(const std::vector &lines) {
-  EditLineStringStreamType combined_stream;
-  for (EditLineStringType line : lines) {
+std::string CombineLines(const std::vector &lines) {
+  std::stringstream combined_stream;
+  for (const std::string &line : lines) {
 combined_stream << line.c_str() << "\n";
   }
   return combined_stream.str();
 }
 
-std::vector SplitLines(const EditLineStringType &input) {
-  std::vector result;
+std::vector SplitLines(const std::string &input) {
+  std::vector result;
   size_t start = 0;
   while (start < input.length()) {
 size_t end = input.find('\n', start);
@@ -161,23 +133,18 @@
   return result;
 }
 
-EditLineStringType FixIndentation(const EditLineStringType &line,
-  int indent_correction) {
+std::string FixIndentation(const std::string &line, int indent_correction) {
   if (indent_correction == 0)
 return line;
   if (indent_correction < 0)
 return line.substr(-indent_correction);
-  return EditLineStringType(indent_correction, EditLineCharType(' ')) + line;
+  return std::string(indent_correction, ' ') + line;
 }
 
-int GetIndentation(const EditLineStringType &line) {
-  int space_count = 0;
-  for (EditLineCharType ch : line) {
-if (ch != EditLineCharType(' '))
-  break;
-++space_count;
-  }
-  return space_count;
+int GetIndentation(const std::string &line) {
+  auto firstNonSpace = std::find_if(line.begin(), line.end(),
+[](const char ch) { return ch != ' '; });
+  return firstNonSpace - line.begin();
 }
 
 bool IsInputPending(FILE *file) {
@@ -206,10 +173,10 @@
   // these objects
   EditlineHistory(const std::string &prefix, uint32_t size, bool unique_entries)
   : m_history(nullptr), m_event(), m_prefix(prefix), m_path() {
-m_history = history_winit();
-history_w(m_history, &m_event, H_SETSIZE, size);
+m_history = history_init();
+history(m_history, &m_event, H_SETSIZE, size);
 if (unique_entries)
-  history_w(m_history, &m_event, H_SETUNIQUE, 1);
+  history(m_history, &m_event, H_SETUNIQUE, 1);
   }
 
   const char *GetHistoryFilePath() {
@@ -222,11 +189,7 @@
   // LLDB stores its history in ~/.lldb/. If for some reason this directory
   // isn't writable or cannot be created, history won't be available.
   if (!llvm::sys::fs::create_directory(lldb_history_file)) {
-#if LLDB_EDITLINE_USE_WCHAR
-std::string filename = m_prefix + "-widehistory";
-#else
 std::string filename = m_prefix + "-history";
-#endif
 llvm::sys::path::append(lldb_history_file, filename);
 m_path = std::string(lldb_history_file.str());
   }
@@ -243,7 +206,7 @@
 Save();
 
 if (m_history) {
-  history_wend(m_history);
+  history_end(m_history);
   m_history = nullptr;
 }
   }
@@ -268,18 +231,18 @@
 
   bool IsValid() const { return m_history != nullptr; }
 
-  HistoryW *GetHistoryPtr() { return m

[Lldb-commits] [PATCH] D106035: Remove conditional compilation for WCHAR support in libedit

[Neal via Phabricator via lldb-commits]

nealsid updated this revision to Diff 363358.
nealsid added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106035/new/

https://reviews.llvm.org/D106035

Files:
  lldb/cmake/modules/LLDBConfig.cmake
  lldb/include/lldb/Host/Editline.h
  lldb/source/Host/common/Editline.cpp

Index: lldb/source/Host/common/Editline.cpp
===
--- lldb/source/Host/common/Editline.cpp
+++ lldb/source/Host/common/Editline.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include 
+#include 
 #include 
 
 #include "lldb/Host/Editline.h"
@@ -61,40 +62,12 @@
 #define ANSI_UP_N_ROWS ESCAPE "[%dA"
 #define ANSI_DOWN_N_ROWS ESCAPE "[%dB"
 
-#if LLDB_EDITLINE_USE_WCHAR
-
-#define EditLineConstString(str) L##str
-#define EditLineStringFormatSpec "%ls"
-
-#else
-
 #define EditLineConstString(str) str
 #define EditLineStringFormatSpec "%s"
 
-// use #defines so wide version functions and structs will resolve to old
-// versions for case of libedit not built with wide char support
-#define history_w history
-#define history_winit history_init
-#define history_wend history_end
-#define HistoryW History
-#define HistEventW HistEvent
-#define LineInfoW LineInfo
-
-#define el_wgets el_gets
-#define el_wgetc el_getc
-#define el_wpush el_push
-#define el_wparse el_parse
-#define el_wset el_set
-#define el_wget el_get
-#define el_wline el_line
-#define el_winsertstr el_insertstr
-#define el_wdeletestr el_deletestr
-
-#endif // #if LLDB_EDITLINE_USE_WCHAR
-
-bool IsOnlySpaces(const EditLineStringType &content) {
-  for (wchar_t ch : content) {
-if (ch != EditLineCharType(' '))
+bool IsOnlySpaces(const std::string &content) {
+  for (char ch : content) {
+if (ch != ' ')
   return false;
   }
   return true;
@@ -132,17 +105,16 @@
   llvm_unreachable("Fully covered switch!");
 }
 
-
-EditLineStringType CombineLines(const std::vector &lines) {
-  EditLineStringStreamType combined_stream;
-  for (EditLineStringType line : lines) {
+std::string CombineLines(const std::vector &lines) {
+  std::stringstream combined_stream;
+  for (const std::string &line : lines) {
 combined_stream << line.c_str() << "\n";
   }
   return combined_stream.str();
 }
 
-std::vector SplitLines(const EditLineStringType &input) {
-  std::vector result;
+std::vector SplitLines(const std::string &input) {
+  std::vector result;
   size_t start = 0;
   while (start < input.length()) {
 size_t end = input.find('\n', start);
@@ -161,23 +133,18 @@
   return result;
 }
 
-EditLineStringType FixIndentation(const EditLineStringType &line,
-  int indent_correction) {
+std::string FixIndentation(const std::string &line, int indent_correction) {
   if (indent_correction == 0)
 return line;
   if (indent_correction < 0)
 return line.substr(-indent_correction);
-  return EditLineStringType(indent_correction, EditLineCharType(' ')) + line;
+  return std::string(indent_correction, ' ') + line;
 }
 
-int GetIndentation(const EditLineStringType &line) {
-  int space_count = 0;
-  for (EditLineCharType ch : line) {
-if (ch != EditLineCharType(' '))
-  break;
-++space_count;
-  }
-  return space_count;
+int GetIndentation(const std::string &line) {
+  auto firstNonSpace = std::find_if(line.begin(), line.end(),
+[](const char ch) { return ch != ' '; });
+  return firstNonSpace - line.begin();
 }
 
 bool IsInputPending(FILE *file) {
@@ -206,10 +173,10 @@
   // these objects
   EditlineHistory(const std::string &prefix, uint32_t size, bool unique_entries)
   : m_history(nullptr), m_event(), m_prefix(prefix), m_path() {
-m_history = history_winit();
-history_w(m_history, &m_event, H_SETSIZE, size);
+m_history = history_init();
+history(m_history, &m_event, H_SETSIZE, size);
 if (unique_entries)
-  history_w(m_history, &m_event, H_SETUNIQUE, 1);
+  history(m_history, &m_event, H_SETUNIQUE, 1);
   }
 
   const char *GetHistoryFilePath() {
@@ -222,11 +189,7 @@
   // LLDB stores its history in ~/.lldb/. If for some reason this directory
   // isn't writable or cannot be created, history won't be available.
   if (!llvm::sys::fs::create_directory(lldb_history_file)) {
-#if LLDB_EDITLINE_USE_WCHAR
-std::string filename = m_prefix + "-widehistory";
-#else
 std::string filename = m_prefix + "-history";
-#endif
 llvm::sys::path::append(lldb_history_file, filename);
 m_path = std::string(lldb_history_file.str());
   }
@@ -243,7 +206,7 @@
 Save();
 
 if (m_history) {
-  history_wend(m_history);
+  history_end(m_history);
   m_history = nullptr;
 }
   }
@@ -268,18 +231,18 @@
 
   bool IsValid() const { return m_histo

[Lldb-commits] [PATCH] D106035: Remove conditional compilation for WCHAR support in libedit

[Neal via Phabricator via lldb-commits]

nealsid added a comment.

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

Definitely +1 on baking it at HEAD for a bit - SGTM.

>>> Also I'm kinda curious if you found any docs/examples that explain whether 
>>> mixing the wchar/char functions like we do now is actually supported in 
>>> libedit? IIUC we call now `el_wset` and `el_set` on the same libedit 
>>> instance. It feels like the `wchar` support in the FreeBSD port was some 
>>> kind of parallel implementation to the normal `char` support, so I'm 
>>> surprised that we can just mix those functions and everything still works 
>>> fine (at least on my Linux machine this seems to work).
>>
>> Yeah, the original source converts the parameters and calls el_w* functions 
>> when the narrow-char functions are called.  This is also true on FreeBSD: 
>> https://github.com/freebsd/freebsd-src/blob/373ffc62c158e52cde86a5b934ab4a51307f9f2e/contrib/libedit/eln.c#L359
>
> Cool, thanks for finding that out!
>
> Anyway, I think this LGTM. Thanks for doing it! Also I can't recall if you 
> have commit access, so please let me know if I should land this.

I don't, but maybe I can start that conversation around commit access.  For 
this patch, I'll watch for the next release branch cut and ask you about 
landing it then.  Thanks!

Neal


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106035/new/

https://reviews.llvm.org/D106035

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106035: Remove conditional compilation for WCHAR support in libedit

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

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

>> Also I'm kinda curious if you found any docs/examples that explain whether 
>> mixing the wchar/char functions like we do now is actually supported in 
>> libedit? IIUC we call now `el_wset` and `el_set` on the same libedit 
>> instance. It feels like the `wchar` support in the FreeBSD port was some 
>> kind of parallel implementation to the normal `char` support, so I'm 
>> surprised that we can just mix those functions and everything still works 
>> fine (at least on my Linux machine this seems to work).
>
> Yeah, the original source converts the parameters and calls el_w* functions 
> when the narrow-char functions are called.  This is also true on FreeBSD: 
> https://github.com/freebsd/freebsd-src/blob/373ffc62c158e52cde86a5b934ab4a51307f9f2e/contrib/libedit/eln.c#L359

Cool, thanks for finding that out!

Anyway, I think this LGTM. Thanks for doing it! Also I can't recall if you have 
commit access, so please let me know if I should land this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106035/new/

https://reviews.llvm.org/D106035

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106035: Remove conditional compilation for WCHAR support in libedit

[Neal via Phabricator via lldb-commits]

nealsid updated this revision to Diff 359533.
nealsid added a comment.

Reran clang-format and removed extra braces.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106035/new/

https://reviews.llvm.org/D106035

Files:
  lldb/cmake/modules/LLDBConfig.cmake
  lldb/include/lldb/Host/Editline.h
  lldb/source/Host/common/Editline.cpp

Index: lldb/source/Host/common/Editline.cpp
===
--- lldb/source/Host/common/Editline.cpp
+++ lldb/source/Host/common/Editline.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include 
+#include 
 #include 
 
 #include "lldb/Host/Editline.h"
@@ -61,40 +62,12 @@
 #define ANSI_UP_N_ROWS ESCAPE "[%dA"
 #define ANSI_DOWN_N_ROWS ESCAPE "[%dB"
 
-#if LLDB_EDITLINE_USE_WCHAR
-
-#define EditLineConstString(str) L##str
-#define EditLineStringFormatSpec "%ls"
-
-#else
-
 #define EditLineConstString(str) str
 #define EditLineStringFormatSpec "%s"
 
-// use #defines so wide version functions and structs will resolve to old
-// versions for case of libedit not built with wide char support
-#define history_w history
-#define history_winit history_init
-#define history_wend history_end
-#define HistoryW History
-#define HistEventW HistEvent
-#define LineInfoW LineInfo
-
-#define el_wgets el_gets
-#define el_wgetc el_getc
-#define el_wpush el_push
-#define el_wparse el_parse
-#define el_wset el_set
-#define el_wget el_get
-#define el_wline el_line
-#define el_winsertstr el_insertstr
-#define el_wdeletestr el_deletestr
-
-#endif // #if LLDB_EDITLINE_USE_WCHAR
-
-bool IsOnlySpaces(const EditLineStringType &content) {
-  for (wchar_t ch : content) {
-if (ch != EditLineCharType(' '))
+bool IsOnlySpaces(const std::string &content) {
+  for (char ch : content) {
+if (ch != ' ')
   return false;
   }
   return true;
@@ -132,17 +105,16 @@
   llvm_unreachable("Fully covered switch!");
 }
 
-
-EditLineStringType CombineLines(const std::vector &lines) {
-  EditLineStringStreamType combined_stream;
-  for (EditLineStringType line : lines) {
+std::string CombineLines(const std::vector &lines) {
+  std::stringstream combined_stream;
+  for (const std::string &line : lines) {
 combined_stream << line.c_str() << "\n";
   }
   return combined_stream.str();
 }
 
-std::vector SplitLines(const EditLineStringType &input) {
-  std::vector result;
+std::vector SplitLines(const std::string &input) {
+  std::vector result;
   size_t start = 0;
   while (start < input.length()) {
 size_t end = input.find('\n', start);
@@ -161,23 +133,18 @@
   return result;
 }
 
-EditLineStringType FixIndentation(const EditLineStringType &line,
-  int indent_correction) {
+std::string FixIndentation(const std::string &line, int indent_correction) {
   if (indent_correction == 0)
 return line;
   if (indent_correction < 0)
 return line.substr(-indent_correction);
-  return EditLineStringType(indent_correction, EditLineCharType(' ')) + line;
+  return std::string(indent_correction, ' ') + line;
 }
 
-int GetIndentation(const EditLineStringType &line) {
-  int space_count = 0;
-  for (EditLineCharType ch : line) {
-if (ch != EditLineCharType(' '))
-  break;
-++space_count;
-  }
-  return space_count;
+int GetIndentation(const std::string &line) {
+  auto firstNonSpace = std::find_if(line.begin(), line.end(),
+[](const char ch) { return ch != ' '; });
+  return firstNonSpace - line.begin();
 }
 
 bool IsInputPending(FILE *file) {
@@ -206,10 +173,10 @@
   // these objects
   EditlineHistory(const std::string &prefix, uint32_t size, bool unique_entries)
   : m_history(nullptr), m_event(), m_prefix(prefix), m_path() {
-m_history = history_winit();
-history_w(m_history, &m_event, H_SETSIZE, size);
+m_history = history_init();
+history(m_history, &m_event, H_SETSIZE, size);
 if (unique_entries)
-  history_w(m_history, &m_event, H_SETUNIQUE, 1);
+  history(m_history, &m_event, H_SETUNIQUE, 1);
   }
 
   const char *GetHistoryFilePath() {
@@ -222,11 +189,7 @@
   // LLDB stores its history in ~/.lldb/. If for some reason this directory
   // isn't writable or cannot be created, history won't be available.
   if (!llvm::sys::fs::create_directory(lldb_history_file)) {
-#if LLDB_EDITLINE_USE_WCHAR
-std::string filename = m_prefix + "-widehistory";
-#else
 std::string filename = m_prefix + "-history";
-#endif
 llvm::sys::path::append(lldb_history_file, filename);
 m_path = std::string(lldb_history_file.str());
   }
@@ -243,7 +206,7 @@
 Save();
 
 if (m_history) {
-  history_wend(m_history);
+  history_end(m_history);
   m_history = nullptr;
 }
   }
@@ -268,18 +231,18 @@
 
   bool IsValid() const { return m_history != nullptr; }
 
-  HistoryW *GetHistoryPtr() { return m_history; }
+

[Lldb-commits] [PATCH] D106035: Remove conditional compilation for WCHAR support in libedit

[Neal via Phabricator via lldb-commits]

nealsid added a comment.

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.

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?

> Also I'm kinda curious if you found any docs/examples that explain whether 
> mixing the wchar/char functions like we do now is actually supported in 
> libedit? IIUC we call now `el_wset` and `el_set` on the same libedit 
> instance. It feels like the `wchar` support in the FreeBSD port was some kind 
> of parallel implementation to the normal `char` support, so I'm surprised 
> that we can just mix those functions and everything still works fine (at 
> least on my Linux machine this seems to work).

Yeah, the original source converts the parameters and calls el_w* functions 
when the narrow-char functions are called.  This is also true on FreeBSD: 
https://github.com/freebsd/freebsd-src/blob/373ffc62c158e52cde86a5b934ab4a51307f9f2e/contrib/libedit/eln.c#L359


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106035/new/

https://reviews.llvm.org/D106035

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106035: Remove conditional compilation for WCHAR support in libedit

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

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.

Also I'm kinda curious if you found any docs/examples that explain whether 
mixing the wchar/char functions like we do now is actually supported in 
libedit? IIUC we call now `el_wset` and `el_set` on the same libedit instance. 
It feels like the `wchar` support in the FreeBSD port was some kind of parallel 
implementation to the normal `char` support, so I'm surprised that we can just 
mix those functions and everything still works fine (at least on my Linux 
machine this seems to work).




Comment at: lldb/source/Host/common/Editline.cpp:554
 if (read_count) {
-  if (CompleteCharacter(ch, *c))
+  if (CompleteCharacter(ch, *c)) {
 return 1;

The original code was actually LLVM-code style (no {} around single line ifs)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106035/new/

https://reviews.llvm.org/D106035

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106035: Remove conditional compilation for WCHAR support in libedit

[Neal via Phabricator via lldb-commits]

nealsid created this revision.
nealsid added a reviewer: LLDB.
nealsid added a project: LLDB.
Herald added subscribers: JDevlieghere, mgorny.
nealsid requested review of this revision.
Herald added a subscriber: lldb-commits.

This change moves to using narrow character types and libedit APIs, because 
those are the same types that the rest of LLVM/LLDB uses, and it's generally 
considered better practice to use UTF-8 encoded in char than it is to use wider 
characters.  However, for character input, the change leaves in using a wchar 
to enable input of multi-byte characters.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106035

Files:
  lldb/cmake/modules/LLDBConfig.cmake
  lldb/include/lldb/Host/Editline.h
  lldb/source/Host/common/Editline.cpp

Index: lldb/source/Host/common/Editline.cpp
===
--- lldb/source/Host/common/Editline.cpp
+++ lldb/source/Host/common/Editline.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include 
+#include 
 #include 
 
 #include "lldb/Host/Editline.h"
@@ -61,40 +62,12 @@
 #define ANSI_UP_N_ROWS ESCAPE "[%dA"
 #define ANSI_DOWN_N_ROWS ESCAPE "[%dB"
 
-#if LLDB_EDITLINE_USE_WCHAR
-
-#define EditLineConstString(str) L##str
-#define EditLineStringFormatSpec "%ls"
-
-#else
-
 #define EditLineConstString(str) str
 #define EditLineStringFormatSpec "%s"
 
-// use #defines so wide version functions and structs will resolve to old
-// versions for case of libedit not built with wide char support
-#define history_w history
-#define history_winit history_init
-#define history_wend history_end
-#define HistoryW History
-#define HistEventW HistEvent
-#define LineInfoW LineInfo
-
-#define el_wgets el_gets
-#define el_wgetc el_getc
-#define el_wpush el_push
-#define el_wparse el_parse
-#define el_wset el_set
-#define el_wget el_get
-#define el_wline el_line
-#define el_winsertstr el_insertstr
-#define el_wdeletestr el_deletestr
-
-#endif // #if LLDB_EDITLINE_USE_WCHAR
-
-bool IsOnlySpaces(const EditLineStringType &content) {
-  for (wchar_t ch : content) {
-if (ch != EditLineCharType(' '))
+bool IsOnlySpaces(const std::string &content) {
+  for (char ch : content) {
+if (ch != ' ')
   return false;
   }
   return true;
@@ -133,16 +106,16 @@
 }
 
 
-EditLineStringType CombineLines(const std::vector &lines) {
-  EditLineStringStreamType combined_stream;
-  for (EditLineStringType line : lines) {
+std::string CombineLines(const std::vector &lines) {
+  std::stringstream combined_stream;
+  for (const std::string& line : lines) {
 combined_stream << line.c_str() << "\n";
   }
   return combined_stream.str();
 }
 
-std::vector SplitLines(const EditLineStringType &input) {
-  std::vector result;
+std::vector SplitLines(const std::string &input) {
+  std::vector result;
   size_t start = 0;
   while (start < input.length()) {
 size_t end = input.find('\n', start);
@@ -161,23 +134,20 @@
   return result;
 }
 
-EditLineStringType FixIndentation(const EditLineStringType &line,
+std::string FixIndentation(const std::string &line,
   int indent_correction) {
   if (indent_correction == 0)
 return line;
   if (indent_correction < 0)
 return line.substr(-indent_correction);
-  return EditLineStringType(indent_correction, EditLineCharType(' ')) + line;
+  return std::string(indent_correction, ' ') + line;
 }
 
-int GetIndentation(const EditLineStringType &line) {
-  int space_count = 0;
-  for (EditLineCharType ch : line) {
-if (ch != EditLineCharType(' '))
-  break;
-++space_count;
-  }
-  return space_count;
+int GetIndentation(const std::string &line) {
+  auto firstNonSpace = std::find_if(line.begin(), line.end(), [] (const char ch) {
+return ch != ' ';
+  });
+  return firstNonSpace - line.begin();
 }
 
 bool IsInputPending(FILE *file) {
@@ -206,10 +176,10 @@
   // these objects
   EditlineHistory(const std::string &prefix, uint32_t size, bool unique_entries)
   : m_history(nullptr), m_event(), m_prefix(prefix), m_path() {
-m_history = history_winit();
-history_w(m_history, &m_event, H_SETSIZE, size);
+m_history = history_init();
+history(m_history, &m_event, H_SETSIZE, size);
 if (unique_entries)
-  history_w(m_history, &m_event, H_SETUNIQUE, 1);
+  history(m_history, &m_event, H_SETUNIQUE, 1);
   }
 
   const char *GetHistoryFilePath() {
@@ -222,11 +192,7 @@
   // LLDB stores its history in ~/.lldb/. If for some reason this directory
   // isn't writable or cannot be created, history won't be available.
   if (!llvm::sys::fs::create_directory(lldb_history_file)) {
-#if LLDB_EDITLINE_USE_WCHAR
-std::string filename = m_prefix + "-widehistory";
-#else
 std::string filename = m_prefix + "-history";
-#endif
 llvm::sys::path::append(lldb_history_file, filename);
 m_path = std::string(lldb_history_file.str());