[lldb-dev] reviewer requested for D101250

[Neal Sidhwaney via lldb-dev]

Hello,

I put together a small patch for the Editline wrapper in LLDB: 
https://reviews.llvm.org/D101250 

It wraps some of the libedit calls in a helper function to provide type safety 
and remove casts that shouldn’t be needed, since the libedit functions take 
varargs.  I was wondering if someone could review it.  Thanks,

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

[lldb-dev] small Editline wrapper cleanup req for feedback

[Neal Sidhwaney via lldb-dev]

Some comments in 
https://reviews.llvm.org/rGfd89af6880f33ead708abe2f7d88ecb687d4e0d2 
 prompted 
me to look more into potential simplifications of our EditLine wrapper and I 
wanted to run this by anyone who is interested before making the changes.

Right now we set a bunch of callbacks in libedit that are captureless lambdas 
implicitly converted to C function pointers.  The lambdas look up an instance 
of our Editline class and invoke member functions.  The boilerplate that could 
be generated by templates is something like the following:

class Foo { // Imagine this is our Editline class that wraps libedit
public:
  unsigned char Foo1(int ch) {  // These are member functions invoked by 
lambdas we pass to libedit
return 'a';
  }
  unsigned char Bar(int ch) {
return 'b';
  }
  unsigned char Baz(int ch) {
return 'c';
  }
};

typedef unsigned char (*elFnPtr)(EditLine*, int);  // Signature of callbacks 
libedit takes (note Edit__L__ine is libedit, and Edit__l__ine is our wrapper)
typedef unsigned char (Foo::*FooMemPtr)(int ch);   // Signature of member 
functions invoked

template
tuple createEditLineCommandDescriptor(const char* 
command, const char* helpText) {
  return make_tuple(command, helpText, [] (EditLine*, int ch) {
cout << ch;
Foo foo;
((&foo)->*callback)('a');
return (unsigned char) ch;
  });
}

auto editlineCommands = {
  createEditLineCommandDescriptor<&Foo::Foo1>(“Command1", “Command1 help"),
  createEditLineCommandDescriptor<&Foo::Bar>(“Command2", “Command2 help")
};

for (auto editlineCommand : editLineCommands) {
 // call into libedit to add editlineCommand, e.g.:
 el_set(EL_ADDFN, editlineCommand.get<0>(), editLineCommand.get<1>(), 
editLineCommand.get<2>());
}

The pointer to member function is a type parameter because otherwise the 
compiler complains about the lambda needing to capture it, in which case we 
could not pass it to libedit.

I also plan to look into the wchar_t/char preprocessor logic that the original 
comment brought up but then I got distracted by shiny template stuff ;-)

Thanks!

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

Re: [lldb-dev] small Editline wrapper cleanup req for feedback

[Neal Sidhwaney via lldb-dev]

Sounds great.  It’ll be useful to combine the function to describe EditLine 
commands, help text, and their callbacks with the logic to register keyboard 
commands for them, as well, so I’ll do both.  

However, I couldn’t make the member function pointer a function argument, 
because then the lambda has to capture it, which made it uncastable to the 
signature for a libedit callback, a C function pointer.

Thanks,

Neal

> On May 4, 2021, at 4:42 PM, Greg Clayton  wrote:
> 
> As long as the solution matches "EditLine *" (C struct type from edit line 
> library) to back to the C++ instance of "Editline" (lower case ell in "line" 
> from LLDB). It should be easy to do with a template.
> 
> I am fine with any new solution that makes it easier to add new commands. I 
> would rather have a templated function argument over a template argument if 
> possible. I am thinking of something like:
> 
> createEditLineCommandDescriptor(“Command1", “Command1 help", &Foo::Foo1);
> createEditLineCommandDescriptor(“Command2", “Command2 help", &Foo::Bar);
> 
> as I find it more readable.
> 
> Greg
> 
>> On Apr 30, 2021, at 9:35 PM, Neal Sidhwaney via lldb-dev 
>> mailto:lldb-dev@lists.llvm.org>> wrote:
>> 
>> Some comments in 
>> https://reviews.llvm.org/rGfd89af6880f33ead708abe2f7d88ecb687d4e0d2 
>> <https://reviews.llvm.org/rGfd89af6880f33ead708abe2f7d88ecb687d4e0d2> 
>> prompted me to look more into potential simplifications of our EditLine 
>> wrapper and I wanted to run this by anyone who is interested before making 
>> the changes.
>> 
>> Right now we set a bunch of callbacks in libedit that are captureless 
>> lambdas implicitly converted to C function pointers.  The lambdas look up an 
>> instance of our Editline class and invoke member functions.  The boilerplate 
>> that could be generated by templates is something like the following:
>> 
>> class Foo { // Imagine this is our Editline class that wraps libedit
>> public:
>>   unsigned char Foo1(int ch) {  // These are member functions invoked by 
>> lambdas we pass to libedit
>> return 'a';
>>   }
>>   unsigned char Bar(int ch) {
>> return 'b';
>>   }
>>   unsigned char Baz(int ch) {
>> return 'c';
>>   }
>> };
>> 
>> typedef unsigned char (*elFnPtr)(EditLine*, int);  // Signature of callbacks 
>> libedit takes (note Edit__L__ine is libedit, and Edit__l__ine is our wrapper)
>> typedef unsigned char (Foo::*FooMemPtr)(int ch);   // Signature of member 
>> functions invoked
>> 
>> template
>> tuple createEditLineCommandDescriptor(const 
>> char* command, const char* helpText) {
>>   return make_tuple(command, helpText, [] (EditLine*, int ch) {
>> cout << ch;
>> Foo foo;
>> ((&foo)->*callback)('a');
>> return (unsigned char) ch;
>>   });
>> }
>> 
>> auto editlineCommands = {
>>   createEditLineCommandDescriptor<&Foo::Foo1>(“Command1", “Command1 help"),
>>   createEditLineCommandDescriptor<&Foo::Bar>(“Command2", “Command2 help")
>> };
>> 
>> for (auto editlineCommand : editLineCommands) {
>>  // call into libedit to add editlineCommand, e.g.:
>>  el_set(EL_ADDFN, editlineCommand.get<0>(), editLineCommand.get<1>(), 
>> editLineCommand.get<2>());
>> }
>> 
>> The pointer to member function is a type parameter because otherwise the 
>> compiler complains about the lambda needing to capture it, in which case we 
>> could not pass it to libedit.
>> 
>> I also plan to look into the wchar_t/char preprocessor logic that the 
>> original comment brought up but then I got distracted by shiny template 
>> stuff ;-)
>> 
>> Thanks!
>> 
>> Neal
>> ___
>> lldb-dev mailing list
>> lldb-dev@lists.llvm.org <mailto:lldb-dev@lists.llvm.org>
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
> 

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

[lldb-dev] proposed change to remove conditional WCHAR support in libedit wrapper

[Neal Sidhwaney via lldb-dev]

Hello,

I was thinking of removing the conditional compilation to use libedit’s WCHAR 
support on non-Windows platforms.  The conditional preprocessor logic is 
definitely the correct way to support both wide & narrow character APIs, but 
editline has enabled the wide API’s by default for some time, and there might 
be an opportunity today to simplify it.  

I’m thinking that using the wide-character API and converting to 
narrow-character string whenever interacting with the rest of LLDB makes the 
most sense.  One hopefully small issue that the history filename is determined 
based on whether WCHAR is used or not.  So history might not work unless people 
rename their file after using a version with this change.  From what I could 
tell, the format of the file is actually the same whether you use history() or 
history_w(), so it doesn’t have to be converted from narrow chars to wide 
characters, although I only tested that on OS X.

There is a WIP of what this might look like here: 
https://github.com/llvm/llvm-project/compare/main...nealsid:lldb-editline-remove-wchar
 

 However, it uses the narrow character libedit API. But it gives an idea of 
what changes will be necessary.  

Anyone else think this could be useful? Or, conversely, does anyone see 
something that I missed that requires the conditional compilation to remain in? 
Since a broken shell would be bad for everyone, I am not sure what the best way 
is to verify who it might break, i.e. if people who use LLDB from head want to 
test it first, or other platforms to consider besides OS X/Linux/Windows.

Thanks,

Neal


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

Re: [lldb-dev] proposed change to remove conditional WCHAR support in libedit wrapper

[Neal Sidhwaney via lldb-dev]

Thank you for the info and for checking the Linux build. I unfortunately do not 
know much about locale issues on Linux, but I tested your change with mine on 
OS X and it worked for all the inputs I tried.

Neal


> On Jul 11, 2021, at 2:11 AM, Jan Kratochvil  wrote:
> 
> On Sat, 10 Jul 2021 10:51:34 +0200, Neal Sidhwaney via lldb-dev wrote:
>> Anyone else think this could be useful? Or, conversely, does anyone see
>> something that I missed that requires the conditional compilation to remain
>> in?
> 
> Oldest platform Red Hat builds LLDB on is RHEL-7 (and its copies) and it
> already contains el_winsertstr and your branch builds fine there.
>   
> https://copr.fedorainfracloud.org/coprs/jankratochvil/lldb/build/2321456/
> 
> OTOH the wide character does not work there and even not on Fedora 34 x86_64:
> typing: žščř
> (lldb) \U+017E\U+0161\U+010D\U+0159
> error: 'žščř' is not a valid command.
> typing: áéíóůúý
> (lldb) \U+016F
> error: 'ů' is not a valid command.
> 
> While mariadb client works fine on Fedora 34 x86_64:
> MariaDB [(none)]> žščřáéíóůúý;
> ERROR 1064 (42000): You have an error in your SQL syntax; check the manual 
> that corresponds to your MariaDB server version for the right syntax to use 
> near 'žščřáéíóůúý' at line 1
> 
> Do you have an idea what can be wrong in LLDB?
> 
> 
> Thanks,
> Jan 
> 

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