[Lldb-commits] [PATCH] D49963: Preliminary patch to support prompt interpolation

[Neal via Phabricator via lldb-commits]

nealsid accepted this revision.
nealsid added inline comments.
This revision is now accepted and ready to land.



Comment at: include/lldb/Host/Editline.h:187
+  /// Register a callback to retrieve the prompt.
+  void SetPromptCallback(PromptCallbackType callback, void *baton);
+  

zturner wrote:
> I'd love to stop using the `baton` idiom if possible.  can you make this 
> function take an `llvm::function_ref` instead?  Then, 
> in the class, store a `std::function`.  When you call 
> `SetPromptCallback`, write `SetPromptCallback([this](EditLine* L) { return  
> this->PromptCallback(L); });`
Sounds good to me.  I'll split up the changes and sent a patch that migrates to 
std::function & removes baton from all callbacks in Editline first and then 
rebase this one on top of that.


Repository:
  rL LLVM

https://reviews.llvm.org/D49963



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

[Lldb-commits] [PATCH] D49963: Preliminary patch to support prompt interpolation

[Neal via Phabricator via lldb-commits]

nealsid added a comment.

In https://reviews.llvm.org/D49963#1183036, @clayborg wrote:

> This will be very cool. Biggest issues to watch out for is to make sure it 
> doesn't return incorrect values when running for things like the thread 
> count. If you switch to use the 
> "m_debugger.GetCommandInterpreter().GetExecutionContext()" it should solve 
> this by making sure the frame and thread are not filled in when the process 
> is running.  It might also be racy. For example if you say "continue", 
> hopefully the process will be resumed by the time the prompt is asked to 
> refresh itself. Since we get events asynchronously this might be a problem.


Nice - TBH, I haven't used LLDB in awhile so having the prompt displayed while 
the target is running wasn't on my list of test cases, but I'll definitely add 
it.  
Perhaps there could also be an indicator like '*' in the prompt when the 
process is currently running so the user will know it's potentially out of date.


Repository:
  rL LLVM

https://reviews.llvm.org/D49963



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

[Lldb-commits] [PATCH] D50299: Migrate to std::function instead of static member functions for callbacks

[Neal via Phabricator via lldb-commits]

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

Fix 80 char line length violations


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D50299

Files:
  include/lldb/Core/IOHandler.h
  include/lldb/Host/Editline.h
  source/Core/IOHandler.cpp
  source/Host/common/Editline.cpp
  unittests/Editline/EditlineTest.cpp

Index: unittests/Editline/EditlineTest.cpp
===
--- unittests/Editline/EditlineTest.cpp
+++ unittests/Editline/EditlineTest.cpp
@@ -78,8 +78,8 @@
   void ConsumeAllOutput();
 
 private:
-  static bool IsInputComplete(lldb_private::Editline *editline,
-  lldb_private::StringList &lines, void *baton);
+  bool IsInputComplete(lldb_private::Editline *editline,
+   lldb_private::StringList &lines);
 
   std::unique_ptr _editline_sp;
 
@@ -129,7 +129,10 @@
   _editline_sp->SetPrompt("> ");
 
   // Hookup our input complete callback.
-  _editline_sp->SetIsInputCompleteCallback(IsInputComplete, this);
+  auto input_complete_cb = [this] (Editline *editline, StringList &lines) {
+ return this->IsInputComplete(editline, lines);
+   };
+  _editline_sp->SetIsInputCompleteCallback(input_complete_cb);
 }
 
 void EditlineAdapter::CloseInput() {
@@ -190,8 +193,7 @@
 }
 
 bool EditlineAdapter::IsInputComplete(lldb_private::Editline *editline,
-  lldb_private::StringList &lines,
-  void *baton) {
+  lldb_private::StringList &lines) {
   // We'll call ourselves complete if we've received a balanced set of braces.
   int start_block_count = 0;
   int brace_balance = 0;
Index: source/Host/common/Editline.cpp
===
--- source/Host/common/Editline.cpp
+++ source/Host/common/Editline.cpp
@@ -576,8 +576,7 @@
   lines.AppendString(new_line_fragment);
 #endif
 
-  int indent_correction = m_fix_indentation_callback(
-  this, lines, 0, m_fix_indentation_callback_baton);
+  int indent_correction = m_fix_indentation_callback(this, lines, 0);
   new_line_fragment = FixIndentation(new_line_fragment, indent_correction);
   m_revert_cursor_index = GetIndentation(new_line_fragment);
 }
@@ -612,8 +611,7 @@
   info->cursor == info->lastchar) {
 if (m_is_input_complete_callback) {
   auto lines = GetInputAsStringList();
-  if (!m_is_input_complete_callback(this, lines,
-m_is_input_complete_callback_baton)) {
+  if (!m_is_input_complete_callback(this, lines)) {
 return BreakLineCommand(ch);
   }
 
@@ -746,8 +744,7 @@
 if (m_fix_indentation_callback) {
   StringList lines = GetInputAsStringList();
   lines.AppendString("");
-  indentation = m_fix_indentation_callback(
-  this, lines, 0, m_fix_indentation_callback_baton);
+  indentation = m_fix_indentation_callback(this, lines, 0);
 }
 m_input_lines.insert(
 m_input_lines.end(),
@@ -792,8 +789,8 @@
   // Save the edits and determine the correct indentation level
   SaveEditedLine();
   StringList lines = GetInputAsStringList(m_current_line_index + 1);
-  int indent_correction = m_fix_indentation_callback(
-  this, lines, cursor_position, m_fix_indentation_callback_baton);
+  int indent_correction = m_fix_indentation_callback(this, lines,
+ cursor_position);
 
   // If it is already correct no special work is needed
   if (indent_correction == 0)
@@ -854,7 +851,7 @@
 }
 
 unsigned char Editline::TabCommand(int ch) {
-  if (m_completion_callback == nullptr)
+  if (!m_completion_callback)
 return CC_ERROR;
 
   const LineInfo *line_info = el_line(m_editline);
@@ -865,7 +862,7 @@
   line_info->buffer, line_info->cursor, line_info->lastchar,
   0,  // Don't skip any matches (start at match zero)
   -1, // Get all the matches
-  completions, m_completion_callback_baton);
+  completions);
 
   if (num_completions == 0)
 return CC_ERROR;
@@ -1238,27 +1235,6 @@
   return result;
 }
 
-void Editline::SetAutoCompleteCallback(CompleteCallbackType callback,
-   void *baton) {
-  m_completion_callback = callback;
-  m_completion_callback_baton = baton;
-}
-
-void Editline::SetIsInputCompleteCallback(IsInputCompleteCallbackType callback,
-  void *baton) {
-  m_is_input_complete_callback = callback;
-  m_is_input_complete_callback_baton = baton;
-}
-
-bool Editline::SetFixIndentationCallback(FixIndentationCallbackType callback,
- void *baton,
- const char *indent_chars) {
-  m_fix_indentation_callback = callback;
-  m_fix_indentation_callback_baton = baton;
-  m_fix_indentat

[Lldb-commits] [PATCH] D50299: Migrate to llvm::unique_function instead of static member functions for callbacks

[Neal via Phabricator via lldb-commits]

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

Ran clang-format


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D50299

Files:
  include/lldb/Core/IOHandler.h
  include/lldb/Host/Editline.h
  source/Core/IOHandler.cpp
  source/Host/common/Editline.cpp
  unittests/Editline/EditlineTest.cpp

Index: unittests/Editline/EditlineTest.cpp
===
--- unittests/Editline/EditlineTest.cpp
+++ unittests/Editline/EditlineTest.cpp
@@ -78,8 +78,8 @@
   void ConsumeAllOutput();
 
 private:
-  static bool IsInputComplete(lldb_private::Editline *editline,
-  lldb_private::StringList &lines, void *baton);
+  bool IsInputComplete(lldb_private::Editline *editline,
+   lldb_private::StringList &lines);
 
   std::unique_ptr _editline_sp;
 
@@ -129,7 +129,10 @@
   _editline_sp->SetPrompt("> ");
 
   // Hookup our input complete callback.
-  _editline_sp->SetIsInputCompleteCallback(IsInputComplete, this);
+  auto input_complete_cb = [this](Editline *editline, StringList &lines) {
+return this->IsInputComplete(editline, lines);
+  };
+  _editline_sp->SetIsInputCompleteCallback(input_complete_cb);
 }
 
 void EditlineAdapter::CloseInput() {
@@ -190,8 +193,7 @@
 }
 
 bool EditlineAdapter::IsInputComplete(lldb_private::Editline *editline,
-  lldb_private::StringList &lines,
-  void *baton) {
+  lldb_private::StringList &lines) {
   // We'll call ourselves complete if we've received a balanced set of braces.
   int start_block_count = 0;
   int brace_balance = 0;
Index: source/Host/common/Editline.cpp
===
--- source/Host/common/Editline.cpp
+++ source/Host/common/Editline.cpp
@@ -576,8 +576,7 @@
   lines.AppendString(new_line_fragment);
 #endif
 
-  int indent_correction = m_fix_indentation_callback(
-  this, lines, 0, m_fix_indentation_callback_baton);
+  int indent_correction = m_fix_indentation_callback(this, lines, 0);
   new_line_fragment = FixIndentation(new_line_fragment, indent_correction);
   m_revert_cursor_index = GetIndentation(new_line_fragment);
 }
@@ -612,8 +611,7 @@
   info->cursor == info->lastchar) {
 if (m_is_input_complete_callback) {
   auto lines = GetInputAsStringList();
-  if (!m_is_input_complete_callback(this, lines,
-m_is_input_complete_callback_baton)) {
+  if (!m_is_input_complete_callback(this, lines)) {
 return BreakLineCommand(ch);
   }
 
@@ -746,8 +744,7 @@
 if (m_fix_indentation_callback) {
   StringList lines = GetInputAsStringList();
   lines.AppendString("");
-  indentation = m_fix_indentation_callback(
-  this, lines, 0, m_fix_indentation_callback_baton);
+  indentation = m_fix_indentation_callback(this, lines, 0);
 }
 m_input_lines.insert(
 m_input_lines.end(),
@@ -792,8 +789,8 @@
   // Save the edits and determine the correct indentation level
   SaveEditedLine();
   StringList lines = GetInputAsStringList(m_current_line_index + 1);
-  int indent_correction = m_fix_indentation_callback(
-  this, lines, cursor_position, m_fix_indentation_callback_baton);
+  int indent_correction =
+  m_fix_indentation_callback(this, lines, cursor_position);
 
   // If it is already correct no special work is needed
   if (indent_correction == 0)
@@ -854,7 +851,7 @@
 }
 
 unsigned char Editline::TabCommand(int ch) {
-  if (m_completion_callback == nullptr)
+  if (!m_completion_callback)
 return CC_ERROR;
 
   const LineInfo *line_info = el_line(m_editline);
@@ -865,7 +862,7 @@
   line_info->buffer, line_info->cursor, line_info->lastchar,
   0,  // Don't skip any matches (start at match zero)
   -1, // Get all the matches
-  completions, m_completion_callback_baton);
+  completions);
 
   if (num_completions == 0)
 return CC_ERROR;
@@ -1238,27 +1235,6 @@
   return result;
 }
 
-void Editline::SetAutoCompleteCallback(CompleteCallbackType callback,
-   void *baton) {
-  m_completion_callback = callback;
-  m_completion_callback_baton = baton;
-}
-
-void Editline::SetIsInputCompleteCallback(IsInputCompleteCallbackType callback,
-  void *baton) {
-  m_is_input_complete_callback = callback;
-  m_is_input_complete_callback_baton = baton;
-}
-
-bool Editline::SetFixIndentationCallback(FixIndentationCallbackType callback,
- void *baton,
- const char *indent_chars) {
-  m_fix_indentation_callback = callback;
-  m_fix_indentation_callback_baton = baton;
-  m_fix_indentation_callback_chars = indent_chars;
-  return false;
-}
-
 bool Editline::GetLine(std::string &line, bool &interrupte

[Lldb-commits] [PATCH] D115126: [LLDB} Add unit tests for Editline keyboard shortcuts

[Neal via Phabricator via lldb-commits]

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

This patch adds tests that verify keyboard shortcuts are set up for 
multi-line/single-line mode.  Along the way I also added a few other small 
changes:

- Removed usage of shared_ptr in a case where object ownership was not shared.
- Changed a file's open mode to be read/write.
- Removed non-emacs configuration of keyboard shortcuts, because Editline 
config is hardcoded to "emacs" in ConfigureEditor.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D115126

Files:
  lldb/include/lldb/Host/Editline.h
  lldb/source/Host/common/Editline.cpp
  lldb/unittests/Editline/EditlineTest.cpp

Index: lldb/unittests/Editline/EditlineTest.cpp
===
--- lldb/unittests/Editline/EditlineTest.cpp
+++ lldb/unittests/Editline/EditlineTest.cpp
@@ -28,7 +28,10 @@
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/StringList.h"
 
+#define ESCAPE "\x1b"
+
 using namespace lldb_private;
+using namespace testing;
 
 namespace {
 const size_t TIMEOUT_MILLIS = 5000;
@@ -80,6 +83,8 @@
 
   void ConsumeAllOutput();
 
+  const std::string GetEditlineOutput() { return testOutputBuffer.str(); }
+
 private:
   bool IsInputComplete(lldb_private::Editline *editline,
lldb_private::StringList &lines);
@@ -91,6 +96,7 @@
   int _pty_secondary_fd;
 
   std::unique_ptr _el_secondary_file;
+  std::stringstream testOutputBuffer;
 };
 
 EditlineAdapter::EditlineAdapter()
@@ -110,11 +116,21 @@
   EXPECT_THAT_ERROR(_pty.OpenSecondary(O_RDWR), llvm::Succeeded());
   _pty_secondary_fd = _pty.GetSecondaryFileDescriptor();
 
-  _el_secondary_file.reset(new FilePointer(fdopen(_pty_secondary_fd, "rw")));
+  _el_secondary_file.reset(new FilePointer(fdopen(_pty_secondary_fd, "w+")));
   EXPECT_FALSE(nullptr == *_el_secondary_file);
   if (*_el_secondary_file == nullptr)
 return;
 
+  // We have to set the output stream we pass to Editline as not using
+  // buffered I/O.  Otherwise we are missing editline's output when we
+  // close the stream in the keybinding test (i.e. the EOF comes
+  // before data previously written to the stream by editline).  This
+  // behavior isn't as I understand the spec becuse fclose should
+  // flush the stream, but my best guess is that it's some unexpected
+  // interaction with stream I/O and ptys.
+  EXPECT_EQ(setvbuf(*_el_secondary_file, nullptr, _IONBF, 0), 0)
+  << "Could not set editline output stream to use unbuffered I/O.";
+
   // Create an Editline instance.
   _editline_sp.reset(new lldb_private::Editline(
   "gtest editor", *_el_secondary_file, *_el_secondary_file,
@@ -207,7 +223,7 @@
 void EditlineAdapter::ConsumeAllOutput() {
   FilePointer output_file(fdopen(_pty_primary_fd, "r"));
 
-  int ch;
+  char ch;
   while ((ch = fgetc(output_file)) != EOF) {
 #if EDITLINE_TEST_DUMP_OUTPUT
 char display_str[] = {0, 0, 0};
@@ -229,15 +245,15 @@
   break;
 }
 printf(" 0x%02x (%03d) (%s)\n", ch, ch, display_str);
-// putc(ch, stdout);
 #endif
+testOutputBuffer << ch;
   }
 }
 
 class EditlineTestFixture : public ::testing::Test {
   SubsystemRAII subsystems;
   EditlineAdapter _el_adapter;
-  std::shared_ptr _sp_output_thread;
+  std::thread _sp_output_thread;
 
 public:
   static void SetUpTestCase() {
@@ -252,14 +268,13 @@
   return;
 
 // Dump output.
-_sp_output_thread =
-std::make_shared([&] { _el_adapter.ConsumeAllOutput(); });
+_sp_output_thread = std::thread([&] { _el_adapter.ConsumeAllOutput(); });
   }
 
   void TearDown() override {
 _el_adapter.CloseInput();
-if (_sp_output_thread)
-  _sp_output_thread->join();
+if (_sp_output_thread.joinable())
+  _sp_output_thread.join();
   }
 
   EditlineAdapter &GetEditlineAdapter() { return _el_adapter; }
@@ -309,4 +324,133 @@
   EXPECT_THAT(reported_lines, testing::ContainerEq(input_lines));
 }
 
+namespace lldb_private {
+
+// Parameter structure for parameterized tests.
+struct KeybindingTestValue {
+  // A number that is used to name the test, so test output can be
+  // mapped back to a specific input.
+  const std::string testNumber;
+  // Whether this keyboard shortcut is only bound in multi-line mode.
+  bool multilineOnly;
+  // The actual key sequence.
+  const std::string keySequence;
+  // The command the key sequence is supposed to execute.
+  const std::string commandName;
+  // This is initialized to the keySequence by default, but gtest has
+  // some errors when the test name as created by the overloaded
+  // operator<< has embedded newlines.  This is even true when we
+  // specify a custom test name function, as we do below when we
+  // instantiate the test case.  In cases where the keyboard shortcut
+  // has a newline or carriage return, this fiel

[Lldb-commits] [PATCH] D115126: [LLDB] Add unit tests for Editline keyboard shortcuts

[Neal via Phabricator via lldb-commits]

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

Fix casing typo in friend class declaration.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115126

Files:
  lldb/include/lldb/Host/Editline.h
  lldb/source/Host/common/Editline.cpp
  lldb/unittests/Editline/EditlineTest.cpp

Index: lldb/unittests/Editline/EditlineTest.cpp
===
--- lldb/unittests/Editline/EditlineTest.cpp
+++ lldb/unittests/Editline/EditlineTest.cpp
@@ -28,7 +28,10 @@
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/StringList.h"
 
+#define ESCAPE "\x1b"
+
 using namespace lldb_private;
+using namespace testing;
 
 namespace {
 const size_t TIMEOUT_MILLIS = 5000;
@@ -80,6 +83,8 @@
 
   void ConsumeAllOutput();
 
+  const std::string GetEditlineOutput() { return testOutputBuffer.str(); }
+
 private:
   bool IsInputComplete(lldb_private::Editline *editline,
lldb_private::StringList &lines);
@@ -91,6 +96,7 @@
   int _pty_secondary_fd;
 
   std::unique_ptr _el_secondary_file;
+  std::stringstream testOutputBuffer;
 };
 
 EditlineAdapter::EditlineAdapter()
@@ -110,11 +116,21 @@
   EXPECT_THAT_ERROR(_pty.OpenSecondary(O_RDWR), llvm::Succeeded());
   _pty_secondary_fd = _pty.GetSecondaryFileDescriptor();
 
-  _el_secondary_file.reset(new FilePointer(fdopen(_pty_secondary_fd, "rw")));
+  _el_secondary_file.reset(new FilePointer(fdopen(_pty_secondary_fd, "w+")));
   EXPECT_FALSE(nullptr == *_el_secondary_file);
   if (*_el_secondary_file == nullptr)
 return;
 
+  // We have to set the output stream we pass to Editline as not using
+  // buffered I/O.  Otherwise we are missing editline's output when we
+  // close the stream in the keybinding test (i.e. the EOF comes
+  // before data previously written to the stream by editline).  This
+  // behavior isn't as I understand the spec becuse fclose should
+  // flush the stream, but my best guess is that it's some unexpected
+  // interaction with stream I/O and ptys.
+  EXPECT_EQ(setvbuf(*_el_secondary_file, nullptr, _IONBF, 0), 0)
+  << "Could not set editline output stream to use unbuffered I/O.";
+
   // Create an Editline instance.
   _editline_sp.reset(new lldb_private::Editline(
   "gtest editor", *_el_secondary_file, *_el_secondary_file,
@@ -207,7 +223,7 @@
 void EditlineAdapter::ConsumeAllOutput() {
   FilePointer output_file(fdopen(_pty_primary_fd, "r"));
 
-  int ch;
+  char ch;
   while ((ch = fgetc(output_file)) != EOF) {
 #if EDITLINE_TEST_DUMP_OUTPUT
 char display_str[] = {0, 0, 0};
@@ -229,15 +245,15 @@
   break;
 }
 printf(" 0x%02x (%03d) (%s)\n", ch, ch, display_str);
-// putc(ch, stdout);
 #endif
+testOutputBuffer << ch;
   }
 }
 
 class EditlineTestFixture : public ::testing::Test {
   SubsystemRAII subsystems;
   EditlineAdapter _el_adapter;
-  std::shared_ptr _sp_output_thread;
+  std::thread _sp_output_thread;
 
 public:
   static void SetUpTestCase() {
@@ -252,14 +268,13 @@
   return;
 
 // Dump output.
-_sp_output_thread =
-std::make_shared([&] { _el_adapter.ConsumeAllOutput(); });
+_sp_output_thread = std::thread([&] { _el_adapter.ConsumeAllOutput(); });
   }
 
   void TearDown() override {
 _el_adapter.CloseInput();
-if (_sp_output_thread)
-  _sp_output_thread->join();
+if (_sp_output_thread.joinable())
+  _sp_output_thread.join();
   }
 
   EditlineAdapter &GetEditlineAdapter() { return _el_adapter; }
@@ -309,4 +324,133 @@
   EXPECT_THAT(reported_lines, testing::ContainerEq(input_lines));
 }
 
+namespace lldb_private {
+
+// Parameter structure for parameterized tests.
+struct KeybindingTestValue {
+  // A number that is used to name the test, so test output can be
+  // mapped back to a specific input.
+  const std::string testNumber;
+  // Whether this keyboard shortcut is only bound in multi-line mode.
+  bool multilineOnly;
+  // The actual key sequence.
+  const std::string keySequence;
+  // The command the key sequence is supposed to execute.
+  const std::string commandName;
+  // This is initialized to the keySequence by default, but gtest has
+  // some errors when the test name as created by the overloaded
+  // operator<< has embedded newlines.  This is even true when we
+  // specify a custom test name function, as we do below when we
+  // instantiate the test case.  In cases where the keyboard shortcut
+  // has a newline or carriage return, this field in the struct can be
+  // set to something that is printable.
+  const std::string &printableKeySequence = keySequence;
+};
+
+std::ostream &operator<<(std::ostream &os, const KeybindingTestValue &kbtv) {
+  return os << "{" << kbtv.printableKeySequence << "  =>  " << kbtv.commandName
+<< " (multiline only: " << kbtv.multilineOnly << ")}";
+}
+
+// The keyboard shortcuts that we're testing.
+const KeybindingTestValue keySequ

[Lldb-commits] [PATCH] D115126: [LLDB] Add unit tests for Editline keyboard shortcuts

[Neal via Phabricator via lldb-commits]

nealsid added a comment.

In D115126#3173236 , @labath wrote:

> All of the changes seem fine, but it's not clear to me what is the value of 
> the new test. To me, it just seems like a change-detector forcing the two 
> versions of the keybindings to stay in sync. The need for the friend 
> declarations and everything also enforces the feeling that this test is not 
> checking what it should be. Did you actually run into some kind of a 
> problem/bug here that you think this test would have prevented?
>
> Personally, I think it would be more useful if we verified the actual 
> behavior of our editline code. We have some code here, in 
> TestMultilineNavigation.py, and a couple of other places, but it would 
> definitely be useful to have more of those.

Thank you! My thought process on the tests was that the exact binding is user 
facing, and a stray extra character or accidental deletion in the strings that 
represent the key sequences should be tested against, as well as any change 
that caused the shortcut to be registered incorrectly.  If I used the same 
table in the dev & test code, it would not catch the first case, and a lost 
keyboard shortcut can be a visible bug, although, in this case, is easily 
fixable through the .editrc file.

The motivation is another change I have in progress that turns the Editline 
code into a table that stores the command, callback, key sequence, and help 
text in a table and loops over it to add them (which sounds suspiciously like 
this patch, I admit ;-)), and I wanted to make sure I didn't break any existing 
shortcuts when making that change, and some other changes I have planned, such 
as removing the conditional WCHAR support.

I also was on the fence about adding the test classes as friends.  I could have 
exposed the functionality to retrieve a shortcut through our Editline class, 
but the test would be the only client today.

Definitely +1 to having keyboard shortcut tests as part of the Python tests.  
Mind if I take that as a TODO?  I see how the coverage of both kinds of tests 
overlaps, but having the gtest tests can help find bugs earlier.  Thanks again.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115126

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

[Lldb-commits] [PATCH] D113650: [lldb] fix -print-script-interpreter-info on windows

[Neal via Phabricator via lldb-commits]

nealsid added a comment.

In D113650#3168622 , @clayborg wrote:

> I can't build on macOS now. I checked out the source code and tried to do:
>
>   cmake -G Ninja -DCMAKE_BUILD_TYPE:STRING=Debug 
> -DLLVM_ENABLE_ASSERTIONS:BOOL=TRUE 
> -DLLVM_ENABLE_PROJECTS='clang;libcxx;libcxxabi;lldb' 
> -DLLDB_BUILD_FRAMEWORK:BOOL=TRUE -DLLDB_USE_SYSTEM_DEBUGSERVER=ON 
> -DLLDB_EDITLINE_USE_WCHAR=0 -DLLDB_ENABLE_LIBEDIT:BOOL=TRUE 
> -DLLDB_ENABLE_CURSES:BOOL=TRUE -DLLDB_ENABLE_PYTHON:BOOL=TRUE 
> -DLLDB_ENABLE_LIBXML2:BOOL=TRUE -DLLDB_ENABLE_LUA:BOOL=FALSE 
> ../llvm-project/llvm

Random drive-by comment - any particular reason you compile without WCHAR 
support in libedit? I have a patch that removes the conditional support from 
our Editline class (actually it was committed and had to be reverted :P). But I 
was under the impression that nobody was using the non-WCHAR support anymore.  
Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113650

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

[Lldb-commits] [PATCH] D115126: [LLDB] Add unit tests for Editline keyboard shortcuts

[Neal via Phabricator via lldb-commits]

nealsid added a comment.

Random unrelated question, and if I missed docs somewhere, I apologize.  Did I 
forget something in creating the revision that caused Harbormaster to not do 
arc lint or arc test?  https://reviews.llvm.org/B137600. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115126

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

[Lldb-commits] [PATCH] D115126: [LLDB] Add unit tests for Editline keyboard shortcuts

[Neal via Phabricator via lldb-commits]

nealsid updated this revision to Diff 392497.
nealsid edited the summary of this revision.
nealsid added a comment.

Testing ARC, no action necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115126

Files:
  lldb/include/lldb/Host/Editline.h
  lldb/source/Host/common/Editline.cpp
  lldb/unittests/Editline/EditlineTest.cpp

Index: lldb/unittests/Editline/EditlineTest.cpp
===
--- lldb/unittests/Editline/EditlineTest.cpp
+++ lldb/unittests/Editline/EditlineTest.cpp
@@ -17,6 +17,7 @@
 
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include 
 #include 
 #include 
 
@@ -28,7 +29,12 @@
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/StringList.h"
 
+using namespace std::chrono_literals;
+
+#define ESCAPE "\x1b"
+
 using namespace lldb_private;
+using namespace testing;
 
 namespace {
 const size_t TIMEOUT_MILLIS = 5000;
@@ -80,6 +86,12 @@
 
   void ConsumeAllOutput();
 
+  const std::string GetEditlineOutput() {
+std::unique_lock lock(outputReadMutex);
+outputReadCv.wait(lock);
+return testOutputBuffer.str();
+  }
+
 private:
   bool IsInputComplete(lldb_private::Editline *editline,
lldb_private::StringList &lines);
@@ -91,6 +103,9 @@
   int _pty_secondary_fd;
 
   std::unique_ptr _el_secondary_file;
+  std::stringstream testOutputBuffer;
+  std::mutex outputReadMutex;
+  std::condition_variable outputReadCv;
 };
 
 EditlineAdapter::EditlineAdapter()
@@ -110,11 +125,21 @@
   EXPECT_THAT_ERROR(_pty.OpenSecondary(O_RDWR), llvm::Succeeded());
   _pty_secondary_fd = _pty.GetSecondaryFileDescriptor();
 
-  _el_secondary_file.reset(new FilePointer(fdopen(_pty_secondary_fd, "rw")));
+  _el_secondary_file.reset(new FilePointer(fdopen(_pty_secondary_fd, "w+")));
   EXPECT_FALSE(nullptr == *_el_secondary_file);
   if (*_el_secondary_file == nullptr)
 return;
 
+  // We have to set the output stream we pass to Editline as not using
+  // buffered I/O.  Otherwise we are missing editline's output when we
+  // close the stream in the keybinding test (i.e. the EOF comes
+  // before data previously written to the stream by editline).  This
+  // behavior isn't as I understand the spec becuse fclose should
+  // flush the stream, but my best guess is that it's some unexpected
+  // interaction with stream I/O and ptys.
+  EXPECT_EQ(setvbuf(*_el_secondary_file, nullptr, _IONBF, 0), 0)
+  << "Could not set editline output stream to use unbuffered I/O.";
+
   // Create an Editline instance.
   _editline_sp.reset(new lldb_private::Editline(
   "gtest editor", *_el_secondary_file, *_el_secondary_file,
@@ -207,7 +232,7 @@
 void EditlineAdapter::ConsumeAllOutput() {
   FilePointer output_file(fdopen(_pty_primary_fd, "r"));
 
-  int ch;
+  char ch;
   while ((ch = fgetc(output_file)) != EOF) {
 #if EDITLINE_TEST_DUMP_OUTPUT
 char display_str[] = {0, 0, 0};
@@ -229,15 +254,18 @@
   break;
 }
 printf(" 0x%02x (%03d) (%s)\n", ch, ch, display_str);
-// putc(ch, stdout);
 #endif
+if (ch == '\n') {
+  outputReadCv.notify_one();
+}
+testOutputBuffer << ch;
   }
 }
 
 class EditlineTestFixture : public ::testing::Test {
   SubsystemRAII subsystems;
   EditlineAdapter _el_adapter;
-  std::shared_ptr _sp_output_thread;
+  std::thread _sp_output_thread;
 
 public:
   static void SetUpTestCase() {
@@ -252,14 +280,13 @@
   return;
 
 // Dump output.
-_sp_output_thread =
-std::make_shared([&] { _el_adapter.ConsumeAllOutput(); });
+_sp_output_thread = std::thread([&] { _el_adapter.ConsumeAllOutput(); });
   }
 
   void TearDown() override {
 _el_adapter.CloseInput();
-if (_sp_output_thread)
-  _sp_output_thread->join();
+if (_sp_output_thread.joinable())
+  _sp_output_thread.join();
   }
 
   EditlineAdapter &GetEditlineAdapter() { return _el_adapter; }
@@ -309,4 +336,131 @@
   EXPECT_THAT(reported_lines, testing::ContainerEq(input_lines));
 }
 
+namespace lldb_private {
+
+// Parameter structure for parameterized tests.
+struct KeybindingTestValue {
+  // A number that is used to name the test, so test output can be
+  // mapped back to a specific input.
+  const std::string testNumber;
+  // Whether this keyboard shortcut is only bound in multi-line mode.
+  bool multilineOnly;
+  // The actual key sequence.
+  const std::string keySequence;
+  // The command the key sequence is supposed to execute.
+  const std::string commandName;
+  // This is initialized to the keySequence by default, but gtest has
+  // some errors when the test name as created by the overloaded
+  // operator<< has embedded newlines.  This is even true when we
+  // specify a custom test name function, as we do below when we
+  // instantiate the test case.  In cases where the keyboard shortcut
+  // has a newline or carriage return, this field in the struct can be
+  // set to some

[Lldb-commits] [PATCH] D115126: [LLDB] Add unit tests for Editline keyboard shortcuts

[Neal via Phabricator via lldb-commits]

nealsid added a comment.

Thank you, Pavel - will address the specific code feedback in subsequent patch.

In D115126#3178906 , @labath wrote:

> Well.. I would say that the most user-facing aspect is the /action/ that 
> happens after the user pressed the appropriate key. But at the end of the 
> day, that doesn't matter that much -- we can (and do) test non-user-facing 
> interfaces as well. But those tests make most sense when there is some 
> complex logic in the code. Making sure that a table does not change by 
> keeping another copy of the table is the very definition of a change-detector 
> test 
> (https://testing.googleblog.com/2015/01/testing-on-toilet-change-detector-tests.html).
>  Like, it may have some value while you're doing the refactoring, but that 
> value quickly drops to zero (or even becomes negative) after you complete it.

I'd disagree completely that the keypresses & key bindings are not user-facing.

The TOTT is interesting, but the changes I'm protecting against don't fall into 
that category.  One reason is that, if the same table were to be used, a stray 
or inadvertent change could compile, pass tests, and make it to users.

Another reason is that editline's API takes varargs, which means that we rely 
on editline to handle all error cases correctly.  I already ran into one issue 
where el_set(el, EL_BIND, ...) supports printing all bound keys (rather than 
making the caller specify which key to return a binding for), but the arguments 
it expects don't pass initial validation in el_set().

(these links may not be the same version as what ships on macOS or Linux 
distros)

https://salsa.debian.org/debian/libedit/-/blob/master/src/eln.c#L176
https://salsa.debian.org/debian/libedit/-/blob/master/src/map.c#L1315

I've never actually seen tests like the one in TOTT; I had 8 amazing years at 
Google, but one thing that always stuck out culturally was how every variation 
of "hello, world" could be turned into a conference-starting PhD thesis.

> If it was a clean test, I could say "whatever, we can delete it if it becomes 
> a burden" , but I can see several problems with the test (and the friend 
> declaration is not the biggest of them), and I don't think it's worth trying 
> to fix those. If you really want to work on that, then I'm not going to stop 
> you, but I would be also perfectly happy to approve a patch that turns the 
> keybindings into a table without an additional test.

I think it's worth it! It may not be the most important thing to LLDB users but 
there's clearly some code that doesn't get touched very often, and making it 
easier to make changes if requests come along will be a good thing.  Also, for 
my own reasons, it is giving me some familiarity with the LLDB & LLVM code 
bases, especially the common code, before moving onto more core parts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115126

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

[Lldb-commits] [PATCH] D115126: [LLDB] Add unit tests for Editline keyboard shortcuts

[Neal via Phabricator via lldb-commits]

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

Patch to address CR feedback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115126

Files:
  lldb/include/lldb/Host/Editline.h
  lldb/source/Host/common/Editline.cpp
  lldb/unittests/Editline/EditlineTest.cpp

Index: lldb/unittests/Editline/EditlineTest.cpp
===
--- lldb/unittests/Editline/EditlineTest.cpp
+++ lldb/unittests/Editline/EditlineTest.cpp
@@ -17,6 +17,7 @@
 
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include 
 #include 
 #include 
 
@@ -28,7 +29,12 @@
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/StringList.h"
 
+using namespace std::chrono_literals;
+
+#define ESCAPE "\x1b"
+
 using namespace lldb_private;
+using namespace testing;
 
 namespace {
 const size_t TIMEOUT_MILLIS = 5000;
@@ -80,6 +86,8 @@
 
   void ConsumeAllOutput();
 
+  const std::string GetEditlineOutput();
+
 private:
   bool IsInputComplete(lldb_private::Editline *editline,
lldb_private::StringList &lines);
@@ -91,6 +99,7 @@
   int _pty_secondary_fd;
 
   std::unique_ptr _el_secondary_file;
+  std::stringstream testOutputBuffer;
 };
 
 EditlineAdapter::EditlineAdapter()
@@ -110,11 +119,21 @@
   EXPECT_THAT_ERROR(_pty.OpenSecondary(O_RDWR), llvm::Succeeded());
   _pty_secondary_fd = _pty.GetSecondaryFileDescriptor();
 
-  _el_secondary_file.reset(new FilePointer(fdopen(_pty_secondary_fd, "rw")));
+  _el_secondary_file.reset(new FilePointer(fdopen(_pty_secondary_fd, "w+")));
   EXPECT_FALSE(nullptr == *_el_secondary_file);
   if (*_el_secondary_file == nullptr)
 return;
 
+  // We have to set the output stream we pass to Editline as not using
+  // buffered I/O.  Otherwise we are missing editline's output when we
+  // close the stream in the keybinding test (i.e. the EOF comes
+  // before data previously written to the stream by editline).  This
+  // behavior isn't as I understand the spec becuse fclose should
+  // flush the stream, but my best guess is that it's some unexpected
+  // interaction with stream I/O and ptys.
+  EXPECT_EQ(setvbuf(*_el_secondary_file, nullptr, _IONBF, 0), 0)
+  << "Could not set editline output stream to use unbuffered I/O.";
+
   // Create an Editline instance.
   _editline_sp.reset(new lldb_private::Editline(
   "gtest editor", *_el_secondary_file, *_el_secondary_file,
@@ -204,6 +223,10 @@
   return (start_block_count > 0) && (brace_balance == 0);
 }
 
+const std::string EditlineAdapter::GetEditlineOutput() {
+  return testOutputBuffer.str();
+}
+
 void EditlineAdapter::ConsumeAllOutput() {
   FilePointer output_file(fdopen(_pty_primary_fd, "r"));
 
@@ -229,15 +252,15 @@
   break;
 }
 printf(" 0x%02x (%03d) (%s)\n", ch, ch, display_str);
-// putc(ch, stdout);
 #endif
+testOutputBuffer << (char)ch;
   }
 }
 
 class EditlineTestFixture : public ::testing::Test {
   SubsystemRAII subsystems;
   EditlineAdapter _el_adapter;
-  std::shared_ptr _sp_output_thread;
+  std::thread _sp_output_thread;
 
 public:
   static void SetUpTestCase() {
@@ -252,16 +275,17 @@
   return;
 
 // Dump output.
-_sp_output_thread =
-std::make_shared([&] { _el_adapter.ConsumeAllOutput(); });
+_sp_output_thread = std::thread([&] { _el_adapter.ConsumeAllOutput(); });
   }
 
-  void TearDown() override {
+  void EndOutputThread() {
 _el_adapter.CloseInput();
-if (_sp_output_thread)
-  _sp_output_thread->join();
+if (_sp_output_thread.joinable())
+  _sp_output_thread.join();
   }
 
+  void TearDown() override { EndOutputThread(); }
+
   EditlineAdapter &GetEditlineAdapter() { return _el_adapter; }
 };
 
@@ -309,4 +333,124 @@
   EXPECT_THAT(reported_lines, testing::ContainerEq(input_lines));
 }
 
+namespace lldb_private {
+
+// Parameter structure for parameterized tests.
+struct KeybindingTestValue {
+  // A number that is used to name the test, so test output can be
+  // mapped back to a specific input.
+  const std::string testNumber;
+  // Whether this keyboard shortcut is only bound in multi-line mode.
+  const bool multilineOnly;
+  // The actual key sequence.
+  const std::string keySequence;
+  // The command the key sequence is supposed to execute.
+  const std::string commandName;
+  // This is initialized to the keySequence by default, but gtest has
+  // some errors when the test name as created by the overloaded
+  // operator<< has embedded newlines.  This is even true when we
+  // specify a custom test name function, as we do below when we
+  // instantiate the test case.  In cases where the keyboard shortcut
+  // has a newline or carriage return, this field in the struct can be
+  // set to something that is printable.
+  const std::string &printableKeySequence = keySequence;
+};
+
+std::ostream &operator<<(std::ostream &os, const KeybindingTestValue &kbtv) {
+  return os

[Lldb-commits] [PATCH] D115126: [LLDB] Add unit tests for Editline keyboard shortcuts

[Neal via Phabricator via lldb-commits]

nealsid added a comment.

Thanks, Pavel.




Comment at: lldb/unittests/Editline/EditlineTest.cpp:133-139
+  // We have to set the output stream we pass to Editline as not using
+  // buffered I/O.  Otherwise we are missing editline's output when we
+  // close the stream in the keybinding test (i.e. the EOF comes
+  // before data previously written to the stream by editline).  This
+  // behavior isn't as I understand the spec becuse fclose should
+  // flush the stream, but my best guess is that it's some unexpected
+  // interaction with stream I/O and ptys.

labath wrote:
> I find this hard to believe. I think it's more likely that this is working 
> around some problem in the test itself. I can't say what is the precise 
> problem without more investigation, but for example the synchronization 
> around testOutputBuffer seems inadequate. `GetEditlineOutput` could deadlock 
> if the read thread reads the newline before it reaches the `wait` call. It 
> also does not handle spurious wakeups. And the accesses to testOutputBuffer 
> are completely thread-unsafe.
I spent a few hours on this before sending the initial patch; it may be some 
interaction with the file APIs and the Psuedoterminal class we have in LLVM.  I 
could not debug anything it was doing that was leading to this behavior, but 
reliably saw EOF being sent before data that had been written to the stream.  I 
also tried various things like flushing it manually, which did not work, but 
sleeping before closing the secondary FD enabled all data to be read on the 
test side.  If you have some time, I'd be happy to have a gchat or screen 
sharing session and see if we can brainstorm something.

Regarding the synchronization, good call; I built on top of the existing 
threading, which did not require synchronizing reading the output of editline, 
because the actual output was never used.  Now, I just call provide a method to 
call thread::join for tests that need to read the output to ensure it's all 
been written.  



Comment at: lldb/unittests/Editline/EditlineTest.cpp:235
 
-  int ch;
+  char ch;
   while ((ch = fgetc(output_file)) != EOF) {

labath wrote:
> This isn't right. The return type of fgetc is deliberately not `char` so that 
> `EOF` does not conflict with an actual character.
Ah, yeah, I changed it back.  For some reason I thought EOF was sent as an 
actual EOF character like CTRL-D, not a special platform-specific integer.



Comment at: lldb/unittests/Editline/EditlineTest.cpp:345
+  // mapped back to a specific input.
+  const std::string testNumber;
+  // Whether this keyboard shortcut is only bound in multi-line mode.

labath wrote:
> You already have a const on the variable declaration. You don't need to put 
> one on every member as well.
I like const on the struct fields since it expresses the intent (a compile-time 
data structure) better, and, without it, if the array variable decl type was 
changed to remove the const, modifying the fields would be permitted.



Comment at: lldb/unittests/Editline/EditlineTest.cpp:352-359
+  // This is initialized to the keySequence by default, but gtest has
+  // some errors when the test name as created by the overloaded
+  // operator<< has embedded newlines.  This is even true when we
+  // specify a custom test name function, as we do below when we
+  // instantiate the test case.  In cases where the keyboard shortcut
+  // has a newline or carriage return, this field in the struct can be
+  // set to something that is printable.

labath wrote:
> Why not just do the replacements in operator<< ?
I started that approach, but doing it at compile time seems easier because the 
output can be matched to the specific test by looking through the table, 
and...it's less code at runtime :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115126

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

[Lldb-commits] [PATCH] D115126: [LLDB] Add unit tests for Editline keyboard shortcuts

[Neal via Phabricator via lldb-commits]

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

Remove extra header and using decl


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115126

Files:
  lldb/include/lldb/Host/Editline.h
  lldb/source/Host/common/Editline.cpp
  lldb/unittests/Editline/EditlineTest.cpp

Index: lldb/unittests/Editline/EditlineTest.cpp
===
--- lldb/unittests/Editline/EditlineTest.cpp
+++ lldb/unittests/Editline/EditlineTest.cpp
@@ -28,7 +28,10 @@
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/StringList.h"
 
+#define ESCAPE "\x1b"
+
 using namespace lldb_private;
+using namespace testing;
 
 namespace {
 const size_t TIMEOUT_MILLIS = 5000;
@@ -80,6 +83,8 @@
 
   void ConsumeAllOutput();
 
+  const std::string GetEditlineOutput();
+
 private:
   bool IsInputComplete(lldb_private::Editline *editline,
lldb_private::StringList &lines);
@@ -91,6 +96,7 @@
   int _pty_secondary_fd;
 
   std::unique_ptr _el_secondary_file;
+  std::stringstream testOutputBuffer;
 };
 
 EditlineAdapter::EditlineAdapter()
@@ -110,11 +116,21 @@
   EXPECT_THAT_ERROR(_pty.OpenSecondary(O_RDWR), llvm::Succeeded());
   _pty_secondary_fd = _pty.GetSecondaryFileDescriptor();
 
-  _el_secondary_file.reset(new FilePointer(fdopen(_pty_secondary_fd, "rw")));
+  _el_secondary_file.reset(new FilePointer(fdopen(_pty_secondary_fd, "w+")));
   EXPECT_FALSE(nullptr == *_el_secondary_file);
   if (*_el_secondary_file == nullptr)
 return;
 
+  // We have to set the output stream we pass to Editline as not using
+  // buffered I/O.  Otherwise we are missing editline's output when we
+  // close the stream in the keybinding test (i.e. the EOF comes
+  // before data previously written to the stream by editline).  This
+  // behavior isn't as I understand the spec becuse fclose should
+  // flush the stream, but my best guess is that it's some unexpected
+  // interaction with stream I/O and ptys.
+  EXPECT_EQ(setvbuf(*_el_secondary_file, nullptr, _IONBF, 0), 0)
+  << "Could not set editline output stream to use unbuffered I/O.";
+
   // Create an Editline instance.
   _editline_sp.reset(new lldb_private::Editline(
   "gtest editor", *_el_secondary_file, *_el_secondary_file,
@@ -204,6 +220,10 @@
   return (start_block_count > 0) && (brace_balance == 0);
 }
 
+const std::string EditlineAdapter::GetEditlineOutput() {
+  return testOutputBuffer.str();
+}
+
 void EditlineAdapter::ConsumeAllOutput() {
   FilePointer output_file(fdopen(_pty_primary_fd, "r"));
 
@@ -229,15 +249,15 @@
   break;
 }
 printf(" 0x%02x (%03d) (%s)\n", ch, ch, display_str);
-// putc(ch, stdout);
 #endif
+testOutputBuffer << (char)ch;
   }
 }
 
 class EditlineTestFixture : public ::testing::Test {
   SubsystemRAII subsystems;
   EditlineAdapter _el_adapter;
-  std::shared_ptr _sp_output_thread;
+  std::thread _sp_output_thread;
 
 public:
   static void SetUpTestCase() {
@@ -252,16 +272,17 @@
   return;
 
 // Dump output.
-_sp_output_thread =
-std::make_shared([&] { _el_adapter.ConsumeAllOutput(); });
+_sp_output_thread = std::thread([&] { _el_adapter.ConsumeAllOutput(); });
   }
 
-  void TearDown() override {
+  void EndOutputThread() {
 _el_adapter.CloseInput();
-if (_sp_output_thread)
-  _sp_output_thread->join();
+if (_sp_output_thread.joinable())
+  _sp_output_thread.join();
   }
 
+  void TearDown() override { EndOutputThread(); }
+
   EditlineAdapter &GetEditlineAdapter() { return _el_adapter; }
 };
 
@@ -309,4 +330,124 @@
   EXPECT_THAT(reported_lines, testing::ContainerEq(input_lines));
 }
 
+namespace lldb_private {
+
+// Parameter structure for parameterized tests.
+struct KeybindingTestValue {
+  // A number that is used to name the test, so test output can be
+  // mapped back to a specific input.
+  const std::string testNumber;
+  // Whether this keyboard shortcut is only bound in multi-line mode.
+  const bool multilineOnly;
+  // The actual key sequence.
+  const std::string keySequence;
+  // The command the key sequence is supposed to execute.
+  const std::string commandName;
+  // This is initialized to the keySequence by default, but gtest has
+  // some errors when the test name as created by the overloaded
+  // operator<< has embedded newlines.  This is even true when we
+  // specify a custom test name function, as we do below when we
+  // instantiate the test case.  In cases where the keyboard shortcut
+  // has a newline or carriage return, this field in the struct can be
+  // set to something that is printable.
+  const std::string &printableKeySequence = keySequence;
+};
+
+std::ostream &operator<<(std::ostream &os, const KeybindingTestValue &kbtv) {
+  return os << "{" << kbtv.printableKeySequence << "  =>  " << kbtv.commandName
+<< " (multiline only: " << kbtv.multilineOnly << ")}";
+}
+
+

[Lldb-commits] [PATCH] D115474: [lldb] Refactor & update of EditlineTest

[Neal via Phabricator via lldb-commits]

nealsid created this revision.
Herald added a subscriber: kristof.beyls.
nealsid requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch has a few minor cleanups and a restructuring of the
EditlineTest class:

Cleanups:

- Adds some comments to member variables
- Renames _editline_sp to _editline_up to reflect that it's a unique_ptr
- Create a debug macro for ConsumeAllOutput when EDITLINE_DUMP_OUTPUT is 1
- Rename _sp_output_thread to output_read_thread since it's no longer a shared 
pointer.

Restructuring:

- Merge EditlineAdapter and EditlineTestFixture.  EditLineAdapter is a

useful separation, but is only used by EditlineTestFixture.  It's a
perfectly fine pattern to put test state & helper methods in the test
fixture class, which is what this patch does.

- Remove FilePointer, which is an RAII class for FILE* pointers.  We

used it in two locations, which are to store FILE* variables for the
pty file handles.  In the primary case, the Pseudoterminal class already
closes the handle in it's destructor, so we don't need to (and were
actually causing a harmless double close of the handle), and in the
second case, we need to manage the handle close ourselves anyway,
because some tests need to close the handle manually as part of their
test case.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D115474

Files:
  lldb/unittests/Editline/EditlineTest.cpp

Index: lldb/unittests/Editline/EditlineTest.cpp
===
--- lldb/unittests/Editline/EditlineTest.cpp
+++ lldb/unittests/Editline/EditlineTest.cpp
@@ -12,6 +12,12 @@
 
 #define EDITLINE_TEST_DUMP_OUTPUT 0
 
+#if EDITLINE_TEST_DUMP_OUTPUT
+#define DEBUG_PRINT_EDITLINE_OUTPUT(ch) PrintEditlineOutput(ch)
+#else
+#define DEBUG_PRINT_EDITLINE_OUTPUT(...)
+#endif
+
 #include 
 #include 
 
@@ -37,40 +43,40 @@
 const size_t TIMEOUT_MILLIS = 5000;
 }
 
-class FilePointer {
+class EditlineTestFixture : public ::testing::Test {
 public:
-  FilePointer() = delete;
-
-  FilePointer(const FilePointer &) = delete;
+  static void SetUpTestCase() {
+// We need a TERM set properly for editline to work as expected.
+setenv("TERM", "vt100", 1);
+  }
 
-  FilePointer(FILE *file_p) : _file_p(file_p) {}
+  void SetUp() override;
 
-  ~FilePointer() {
-if (_file_p != nullptr) {
-  const int close_result = fclose(_file_p);
-  EXPECT_EQ(0, close_result);
-}
-  }
+protected:
+  // EditLine callback for us to tell whether input is complete or
+  // not.
+  bool IsInputComplete(lldb_private::Editline *editline,
+   lldb_private::StringList &lines);
 
-  operator FILE *() { return _file_p; }
+  // Helper debug method to escape & print libedit's output.
+  void PrintEditlineOutput(char ch);
 
-private:
-  FILE *_file_p;
-};
+  // This is normally executed during test case teardown, but some
+  // cases call it explicitly to ensure that all libeditoutput is read
+  // before verifying it.
+  void EndOutputThread() {
+CloseInput();
+if (output_read_thread.joinable())
+  output_read_thread.join();
+  }
 
-/**
- Wraps an Editline class, providing a simple way to feed
- input (as if from the keyboard) and receive output from Editline.
- */
-class EditlineAdapter {
-public:
-  EditlineAdapter();
+  void TearDown() override { EndOutputThread(); }
 
+  // Close the file pointer that libedit outputs to (which is our
+  // input).
   void CloseInput();
 
-  bool IsValid() const { return _editline_sp != nullptr; }
-
-  lldb_private::Editline &GetEditline() { return *_editline_sp; }
+  lldb_private::Editline &GetEditline() { return *_editline_up; }
 
   bool SendLine(const std::string &line);
 
@@ -86,216 +92,36 @@
   const std::string GetEditlineOutput();
 
 private:
-  bool IsInputComplete(lldb_private::Editline *editline,
-   lldb_private::StringList &lines);
+  // EditLine needs a filesystem for reading the history file.
+  SubsystemRAII subsystems;
 
-  std::unique_ptr _editline_sp;
+  // A thread to read libedit's stdout.
+  std::thread output_read_thread;
+  // The EditLine instance under test.
+  std::unique_ptr _editline_up;
 
+  // Pseudoterminal for libedit's stdout.
   PseudoTerminal _pty;
+  // Primary/secondary file descriptors for the pty.
   int _pty_primary_fd;
   int _pty_secondary_fd;
 
-  std::unique_ptr _el_secondary_file;
+  // A FILE* stream that is passed to libedit for stdout.
+  FILE *_el_secondary_file;
+  // The buffer the thread above stores libedit's output into.
   std::stringstream testOutputBuffer;
 };
 
-EditlineAdapter::EditlineAdapter()
-: _editline_sp(), _pty(), _pty_primary_fd(-1), _pty_secondary_fd(-1),
-  _el_secondary_file() {
-  lldb_private::Status error;
-
-  // Open the first primary pty available.
-  EXPECT_THAT_ERROR(_pty.OpenFirstAvailablePrimary(O_RDWR), llvm::Succeeded());
-
-  // Grab the primary fd.  This is a file descriptor we 

[Lldb-commits] [PATCH] D115126: [LLDB] Add unit tests for Editline keyboard shortcuts

[Neal via Phabricator via lldb-commits]

nealsid added a comment.

Thanks, Pavel.  Some comments responded to, will take another look at the 
buffered/unbuffered I/O issue and address the remaining comments.




Comment at: lldb/include/lldb/Host/Editline.h:392-393
+
+  friend class EditlineKeyboardBindingTest_MultiLineEditlineKeybindings_Test;
+  friend class EditlineKeyboardBindingTest_SingleLineEditlineKeybindings_Test;
 };

labath wrote:
> This also isn't something we normally do. I'd recommend just making a 
> function like `DumpKeyBindings()` or something. It's true that it would only 
> be used in tests right now, but I can imagine some day adding an lldb command 
> to dump current bindings, so it does not seem completely out of place. If you 
> wanted to, you could even add it right now and do your test that way.
Can you clarify this? I see a lot of friend class declarations in the LLDB 
codebase, both prod and test.

A command to dump key bindings could be useful in the future, but the 
requirement of parsing editline's stdout and manipulating it into some sort of 
structure for an API sounds like a maintenance tax until that command is 
implemented.



Comment at: lldb/unittests/Editline/EditlineTest.cpp:133-139
+  // We have to set the output stream we pass to Editline as not using
+  // buffered I/O.  Otherwise we are missing editline's output when we
+  // close the stream in the keybinding test (i.e. the EOF comes
+  // before data previously written to the stream by editline).  This
+  // behavior isn't as I understand the spec becuse fclose should
+  // flush the stream, but my best guess is that it's some unexpected
+  // interaction with stream I/O and ptys.

labath wrote:
> nealsid wrote:
> > labath wrote:
> > > I find this hard to believe. I think it's more likely that this is 
> > > working around some problem in the test itself. I can't say what is the 
> > > precise problem without more investigation, but for example the 
> > > synchronization around testOutputBuffer seems inadequate. 
> > > `GetEditlineOutput` could deadlock if the read thread reads the newline 
> > > before it reaches the `wait` call. It also does not handle spurious 
> > > wakeups. And the accesses to testOutputBuffer are completely 
> > > thread-unsafe.
> > I spent a few hours on this before sending the initial patch; it may be 
> > some interaction with the file APIs and the Psuedoterminal class we have in 
> > LLVM.  I could not debug anything it was doing that was leading to this 
> > behavior, but reliably saw EOF being sent before data that had been written 
> > to the stream.  I also tried various things like flushing it manually, 
> > which did not work, but sleeping before closing the secondary FD enabled 
> > all data to be read on the test side.  If you have some time, I'd be happy 
> > to have a gchat or screen sharing session and see if we can brainstorm 
> > something.
> > 
> > Regarding the synchronization, good call; I built on top of the existing 
> > threading, which did not require synchronizing reading the output of 
> > editline, because the actual output was never used.  Now, I just call 
> > provide a method to call thread::join for tests that need to read the 
> > output to ensure it's all been written.  
> Well.. I tried running this locally, and the test passed just fine even with 
> this code deleted. I don't think we should commit code like this without 
> understanding the problem in more detail. (Like, if it's a problem with llvm 
> code, then lets fix that; if it's a problem with libc code (I doubt that), 
> then let's file a bug before working around it.)If you want to discuss this 
> in a more interactive fashion, you can find me on the `#lldb` channel @ 
> irc.oftc.net.
Cool, I'll take another look..what platform were you running on? 



Comment at: lldb/unittests/Editline/EditlineTest.cpp:345
+  // mapped back to a specific input.
+  const std::string testNumber;
+  // Whether this keyboard shortcut is only bound in multi-line mode.

labath wrote:
> nealsid wrote:
> > labath wrote:
> > > You already have a const on the variable declaration. You don't need to 
> > > put one on every member as well.
> > I like const on the struct fields since it expresses the intent (a 
> > compile-time data structure) better, and, without it, if the array variable 
> > decl type was changed to remove the const, modifying the fields would be 
> > permitted.
> Well, we definitely have have different views on redundancy. I could write a 
> PhD thesis on why I think this is wrong, but right now, let me just say this: 
> it isn't consistent with how we define const structs anywhere else in llvm or 
> lldb. Here's some examples: 
> ,
>  
> 

[Lldb-commits] [PATCH] D104437: Add test for functions with extended characters.

[Neal via Phabricator via lldb-commits]

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

This small patch adds a test for functions & lldb commands with unicode 
characters in them.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104437

Files:
  lldb/test/Shell/Breakpoint/Inputs/unicode-function-name.c
  lldb/test/Shell/Breakpoint/unicode-function-name.test


Index: lldb/test/Shell/Breakpoint/unicode-function-name.test
===
--- /dev/null
+++ lldb/test/Shell/Breakpoint/unicode-function-name.test
@@ -0,0 +1,11 @@
+# RUN: %build% %p/Inputs/unicode-function-name.c --nodefaultlib -o %t
+# RUN: %lldb -b -s %s %t | FileCheck %s
+
+break set -n Δ
+# CHECK: Breakpoint 1:
+
+run
+# CHECK: Process {{[0-9]+}} stopped
+
+print ΔΔ
+# CHECK: (int) $0 = 5
\ No newline at end of file
Index: lldb/test/Shell/Breakpoint/Inputs/unicode-function-name.c
===
--- /dev/null
+++ lldb/test/Shell/Breakpoint/Inputs/unicode-function-name.c
@@ -0,0 +1,6 @@
+int Δ(int ΔΔ) { return ΔΔ; }
+
+int main(int argc, char *argv[]) {
+  int x = Δ(5);
+  return 0;
+}


Index: lldb/test/Shell/Breakpoint/unicode-function-name.test
===
--- /dev/null
+++ lldb/test/Shell/Breakpoint/unicode-function-name.test
@@ -0,0 +1,11 @@
+# RUN: %build% %p/Inputs/unicode-function-name.c --nodefaultlib -o %t
+# RUN: %lldb -b -s %s %t | FileCheck %s
+
+break set -n Δ
+# CHECK: Breakpoint 1:
+
+run
+# CHECK: Process {{[0-9]+}} stopped
+
+print ΔΔ
+# CHECK: (int) $0 = 5
\ No newline at end of file
Index: lldb/test/Shell/Breakpoint/Inputs/unicode-function-name.c
===
--- /dev/null
+++ lldb/test/Shell/Breakpoint/Inputs/unicode-function-name.c
@@ -0,0 +1,6 @@
+int Δ(int ΔΔ) { return ΔΔ; }
+
+int main(int argc, char *argv[]) {
+  int x = Δ(5);
+  return 0;
+}
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D104437: Add test for functions with extended characters.

[Neal via Phabricator via lldb-commits]

nealsid updated this revision to Diff 352612.
nealsid added a comment.
Herald added a subscriber: JDevlieghere.

Add newline.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104437

Files:
  lldb/test/Shell/Breakpoint/Inputs/unicode-function-name.c
  lldb/test/Shell/Breakpoint/unicode-function-name.test


Index: lldb/test/Shell/Breakpoint/unicode-function-name.test
===
--- /dev/null
+++ lldb/test/Shell/Breakpoint/unicode-function-name.test
@@ -0,0 +1,11 @@
+# RUN: %build% %p/Inputs/unicode-function-name.c --nodefaultlib -o %t
+# RUN: %lldb -b -s %s %t | FileCheck %s
+
+break set -n Δ
+# CHECK: Breakpoint 1:
+
+run
+# CHECK: Process {{[0-9]+}} stopped
+
+print ΔΔ
+# CHECK: (int) $0 = 5
Index: lldb/test/Shell/Breakpoint/Inputs/unicode-function-name.c
===
--- /dev/null
+++ lldb/test/Shell/Breakpoint/Inputs/unicode-function-name.c
@@ -0,0 +1,6 @@
+int Δ(int ΔΔ) { return ΔΔ; }
+
+int main(int argc, char *argv[]) {
+  int x = Δ(5);
+  return 0;
+}


Index: lldb/test/Shell/Breakpoint/unicode-function-name.test
===
--- /dev/null
+++ lldb/test/Shell/Breakpoint/unicode-function-name.test
@@ -0,0 +1,11 @@
+# RUN: %build% %p/Inputs/unicode-function-name.c --nodefaultlib -o %t
+# RUN: %lldb -b -s %s %t | FileCheck %s
+
+break set -n Δ
+# CHECK: Breakpoint 1:
+
+run
+# CHECK: Process {{[0-9]+}} stopped
+
+print ΔΔ
+# CHECK: (int) $0 = 5
Index: lldb/test/Shell/Breakpoint/Inputs/unicode-function-name.c
===
--- /dev/null
+++ lldb/test/Shell/Breakpoint/Inputs/unicode-function-name.c
@@ -0,0 +1,6 @@
+int Δ(int ΔΔ) { return ΔΔ; }
+
+int main(int argc, char *argv[]) {
+  int x = Δ(5);
+  return 0;
+}
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D104437: Add test for functions with extended characters.

[Neal via Phabricator via lldb-commits]

nealsid added a comment.

Ah, my grep/find skills clearly failed me :)

Yeah, those tests are exactly the same scenarios.  However, if I understand 
correctly, don't they use the API? I wanted to add some coverage for the shell 
because I'm making changes to the Editline wrapper, and the existing tests 
don't appear to cover user input from the command line.  But maybe I'm missing 
how those two tie together.  Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104437

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

[Lldb-commits] [PATCH] D104437: Add test for functions with extended characters.

[Neal via Phabricator via lldb-commits]

nealsid added a comment.

Thanks to both of you for this! I’m traveling for a few days but will be able 
to take a look towards the end of the week.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104437

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

[Lldb-commits] [PATCH] D104437: Add test for functions with extended characters.

[Neal via Phabricator via lldb-commits]

nealsid added a comment.

Agreed, it looks like there's plenty of test coverage for this scenario, so 
I'll revert this patch - sorry about that.  I'm working on removing the 
wchar/char preprocessor logic in Editline.cpp so I'll verify the existing 
coverage before sending that for review.  Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104437

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

[Lldb-commits] [PATCH] D104437: Add test for functions with extended characters.

[Neal via Phabricator via lldb-commits]

nealsid abandoned this revision.
nealsid added a comment.

Not needed with existing test coverage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104437

___
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());
   

[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

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

[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 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 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

[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

[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

[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] D50299: Migrate to llvm::unique_function instead of static member functions for callbacks

[Neal via Phabricator via lldb-commits]

nealsid updated this revision to Diff 326553.
nealsid added a comment.
Herald added a project: LLDB.

I'm updating this very old patch that I forgot about in 2018.  This one removes 
the use of batons in the edit line callbacks, replaces typedefs with using 
declarations, and uses unique_function instead of callbacks.  
Thanks,
Neal


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D50299

Files:
  lldb/include/lldb/Core/IOHandler.h
  lldb/include/lldb/Host/Editline.h
  lldb/source/Core/IOHandler.cpp
  lldb/source/Host/common/Editline.cpp
  lldb/unittests/Editline/EditlineTest.cpp

Index: lldb/unittests/Editline/EditlineTest.cpp
===
--- lldb/unittests/Editline/EditlineTest.cpp
+++ lldb/unittests/Editline/EditlineTest.cpp
@@ -81,8 +81,8 @@
   void ConsumeAllOutput();
 
 private:
-  static bool IsInputComplete(lldb_private::Editline *editline,
-  lldb_private::StringList &lines, void *baton);
+  bool IsInputComplete(lldb_private::Editline *editline,
+   lldb_private::StringList &lines);
 
   std::unique_ptr _editline_sp;
 
@@ -122,7 +122,10 @@
   _editline_sp->SetPrompt("> ");
 
   // Hookup our input complete callback.
-  _editline_sp->SetIsInputCompleteCallback(IsInputComplete, this);
+  auto input_complete_cb = [this](Editline *editline, StringList &lines) {
+return this->IsInputComplete(editline, lines);
+  };
+  _editline_sp->SetIsInputCompleteCallback(input_complete_cb);
 }
 
 void EditlineAdapter::CloseInput() {
@@ -183,8 +186,7 @@
 }
 
 bool EditlineAdapter::IsInputComplete(lldb_private::Editline *editline,
-  lldb_private::StringList &lines,
-  void *baton) {
+  lldb_private::StringList &lines) {
   // We'll call ourselves complete if we've received a balanced set of braces.
   int start_block_count = 0;
   int brace_balance = 0;
Index: lldb/source/Host/common/Editline.cpp
===
--- lldb/source/Host/common/Editline.cpp
+++ lldb/source/Host/common/Editline.cpp
@@ -641,8 +641,7 @@
   lines.AppendString(new_line_fragment);
 #endif
 
-  int indent_correction = m_fix_indentation_callback(
-  this, lines, 0, m_fix_indentation_callback_baton);
+  int indent_correction = m_fix_indentation_callback(this, lines, 0);
   new_line_fragment = FixIndentation(new_line_fragment, indent_correction);
   m_revert_cursor_index = GetIndentation(new_line_fragment);
 }
@@ -677,8 +676,7 @@
   info->cursor == info->lastchar) {
 if (m_is_input_complete_callback) {
   auto lines = GetInputAsStringList();
-  if (!m_is_input_complete_callback(this, lines,
-m_is_input_complete_callback_baton)) {
+  if (!m_is_input_complete_callback(this, lines)) {
 return BreakLineCommand(ch);
   }
 
@@ -811,8 +809,7 @@
 if (m_fix_indentation_callback) {
   StringList lines = GetInputAsStringList();
   lines.AppendString("");
-  indentation = m_fix_indentation_callback(
-  this, lines, 0, m_fix_indentation_callback_baton);
+  indentation = m_fix_indentation_callback(this, lines, 0);
 }
 m_input_lines.insert(
 m_input_lines.end(),
@@ -857,8 +854,8 @@
   // Save the edits and determine the correct indentation level
   SaveEditedLine();
   StringList lines = GetInputAsStringList(m_current_line_index + 1);
-  int indent_correction = m_fix_indentation_callback(
-  this, lines, cursor_position, m_fix_indentation_callback_baton);
+  int indent_correction =
+  m_fix_indentation_callback(this, lines, cursor_position);
 
   // If it is already correct no special work is needed
   if (indent_correction == 0)
@@ -977,7 +974,7 @@
 }
 
 unsigned char Editline::TabCommand(int ch) {
-  if (m_completion_callback == nullptr)
+  if (!m_completion_callback)
 return CC_ERROR;
 
   const LineInfo *line_info = el_line(m_editline);
@@ -988,7 +985,7 @@
   CompletionResult result;
   CompletionRequest request(line, cursor_index, result);
 
-  m_completion_callback(request, m_completion_callback_baton);
+  m_completion_callback(request);
 
   llvm::ArrayRef results = result.GetResults();
 
@@ -1052,7 +1049,7 @@
line_info->lastchar - line_info->buffer);
 
   if (llvm::Optional to_add =
-  m_suggestion_callback(line, m_suggestion_callback_baton))
+  m_suggestion_callback(line))
 el_insertstr(m_editline, to_add->c_str());
 
   return CC_REDISPLAY;
@@ -1066,7 +1063,7 @@
line_info->lastchar - line_info->buffer);
 
   if (llvm::Optional to_add =
-  m_suggestion_callback(line, m_suggestion_callback_baton)) {
+  m_suggestion_callback(line)) {
 std::string to_add_color = ANSI_FAINT

[Lldb-commits] [PATCH] D50299: Migrate to llvm::unique_function instead of static member functions for callbacks

[Neal via Phabricator via lldb-commits]

nealsid added a comment.

In D50299#2589289 , @JDevlieghere 
wrote:

> Thanks Neal. Can you run clang-format again and upload the diff with context 
> (`git diff -U`). Overall this looks pretty good.

Thanks for the review.  I have run clang-format and am currently running 
through a bunch of testing with the completion/suggestion features just to make 
sure.  Hope to get it updated in the next couple of days.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D50299

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

[Lldb-commits] [PATCH] D50299: Migrate to llvm::unique_function instead of static member functions for callbacks

[Neal via Phabricator via lldb-commits]

nealsid updated this revision to Diff 326954.
nealsid removed a project: LLDB.
nealsid added a comment.

Updated diff with run of clang-format and added a check for function pointer 
validity.  Thanks.


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

https://reviews.llvm.org/D50299

Files:
  lldb/include/lldb/Core/IOHandler.h
  lldb/include/lldb/Host/Editline.h
  lldb/source/Core/IOHandler.cpp
  lldb/source/Host/common/Editline.cpp
  lldb/unittests/Editline/EditlineTest.cpp

Index: lldb/unittests/Editline/EditlineTest.cpp
===
--- lldb/unittests/Editline/EditlineTest.cpp
+++ lldb/unittests/Editline/EditlineTest.cpp
@@ -81,8 +81,8 @@
   void ConsumeAllOutput();
 
 private:
-  static bool IsInputComplete(lldb_private::Editline *editline,
-  lldb_private::StringList &lines, void *baton);
+  bool IsInputComplete(lldb_private::Editline *editline,
+   lldb_private::StringList &lines);
 
   std::unique_ptr _editline_sp;
 
@@ -122,7 +122,10 @@
   _editline_sp->SetPrompt("> ");
 
   // Hookup our input complete callback.
-  _editline_sp->SetIsInputCompleteCallback(IsInputComplete, this);
+  auto input_complete_cb = [this](Editline *editline, StringList &lines) {
+return this->IsInputComplete(editline, lines);
+  };
+  _editline_sp->SetIsInputCompleteCallback(input_complete_cb);
 }
 
 void EditlineAdapter::CloseInput() {
@@ -183,8 +186,7 @@
 }
 
 bool EditlineAdapter::IsInputComplete(lldb_private::Editline *editline,
-  lldb_private::StringList &lines,
-  void *baton) {
+  lldb_private::StringList &lines) {
   // We'll call ourselves complete if we've received a balanced set of braces.
   int start_block_count = 0;
   int brace_balance = 0;
Index: lldb/source/Host/common/Editline.cpp
===
--- lldb/source/Host/common/Editline.cpp
+++ lldb/source/Host/common/Editline.cpp
@@ -641,8 +641,7 @@
   lines.AppendString(new_line_fragment);
 #endif
 
-  int indent_correction = m_fix_indentation_callback(
-  this, lines, 0, m_fix_indentation_callback_baton);
+  int indent_correction = m_fix_indentation_callback(this, lines, 0);
   new_line_fragment = FixIndentation(new_line_fragment, indent_correction);
   m_revert_cursor_index = GetIndentation(new_line_fragment);
 }
@@ -677,8 +676,7 @@
   info->cursor == info->lastchar) {
 if (m_is_input_complete_callback) {
   auto lines = GetInputAsStringList();
-  if (!m_is_input_complete_callback(this, lines,
-m_is_input_complete_callback_baton)) {
+  if (!m_is_input_complete_callback(this, lines)) {
 return BreakLineCommand(ch);
   }
 
@@ -811,8 +809,7 @@
 if (m_fix_indentation_callback) {
   StringList lines = GetInputAsStringList();
   lines.AppendString("");
-  indentation = m_fix_indentation_callback(
-  this, lines, 0, m_fix_indentation_callback_baton);
+  indentation = m_fix_indentation_callback(this, lines, 0);
 }
 m_input_lines.insert(
 m_input_lines.end(),
@@ -857,8 +854,8 @@
   // Save the edits and determine the correct indentation level
   SaveEditedLine();
   StringList lines = GetInputAsStringList(m_current_line_index + 1);
-  int indent_correction = m_fix_indentation_callback(
-  this, lines, cursor_position, m_fix_indentation_callback_baton);
+  int indent_correction =
+  m_fix_indentation_callback(this, lines, cursor_position);
 
   // If it is already correct no special work is needed
   if (indent_correction == 0)
@@ -977,7 +974,7 @@
 }
 
 unsigned char Editline::TabCommand(int ch) {
-  if (m_completion_callback == nullptr)
+  if (!m_completion_callback)
 return CC_ERROR;
 
   const LineInfo *line_info = el_line(m_editline);
@@ -988,7 +985,7 @@
   CompletionResult result;
   CompletionRequest request(line, cursor_index, result);
 
-  m_completion_callback(request, m_completion_callback_baton);
+  m_completion_callback(request);
 
   llvm::ArrayRef results = result.GetResults();
 
@@ -1047,12 +1044,15 @@
 }
 
 unsigned char Editline::ApplyAutosuggestCommand(int ch) {
+  if (!m_suggestion_callback) {
+return CC_REDISPLAY;
+  }
+
   const LineInfo *line_info = el_line(m_editline);
   llvm::StringRef line(line_info->buffer,
line_info->lastchar - line_info->buffer);
 
-  if (llvm::Optional to_add =
-  m_suggestion_callback(line, m_suggestion_callback_baton))
+  if (llvm::Optional to_add = m_suggestion_callback(line))
 el_insertstr(m_editline, to_add->c_str());
 
   return CC_REDISPLAY;
@@ -1061,12 +1061,16 @@
 unsigned char Editline::TypedCharacter(int ch) {
   std::string typed = std::string(1, ch);
   el_insertstr(m_editline, typed.c_str());
+
+  if (!m_suggestion_callback) {
+

[Lldb-commits] [PATCH] D50299: Migrate to llvm::unique_function instead of static member functions for callbacks

[Neal via Phabricator via lldb-commits]

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

Removed comment on #include line and organized the #includes to match coding 
guidelines better (I wasn't sure about whether the project uses blank lines in 
between #includes from different subproject like lldb/clang/llvm so I didn't 
make the change more broadly)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D50299

Files:
  lldb/include/lldb/Core/IOHandler.h
  lldb/include/lldb/Host/Editline.h
  lldb/source/Core/IOHandler.cpp
  lldb/source/Host/common/Editline.cpp
  lldb/unittests/Editline/EditlineTest.cpp

Index: lldb/unittests/Editline/EditlineTest.cpp
===
--- lldb/unittests/Editline/EditlineTest.cpp
+++ lldb/unittests/Editline/EditlineTest.cpp
@@ -81,8 +81,8 @@
   void ConsumeAllOutput();
 
 private:
-  static bool IsInputComplete(lldb_private::Editline *editline,
-  lldb_private::StringList &lines, void *baton);
+  bool IsInputComplete(lldb_private::Editline *editline,
+   lldb_private::StringList &lines);
 
   std::unique_ptr _editline_sp;
 
@@ -122,7 +122,10 @@
   _editline_sp->SetPrompt("> ");
 
   // Hookup our input complete callback.
-  _editline_sp->SetIsInputCompleteCallback(IsInputComplete, this);
+  auto input_complete_cb = [this](Editline *editline, StringList &lines) {
+return this->IsInputComplete(editline, lines);
+  };
+  _editline_sp->SetIsInputCompleteCallback(input_complete_cb);
 }
 
 void EditlineAdapter::CloseInput() {
@@ -183,8 +186,7 @@
 }
 
 bool EditlineAdapter::IsInputComplete(lldb_private::Editline *editline,
-  lldb_private::StringList &lines,
-  void *baton) {
+  lldb_private::StringList &lines) {
   // We'll call ourselves complete if we've received a balanced set of braces.
   int start_block_count = 0;
   int brace_balance = 0;
Index: lldb/source/Host/common/Editline.cpp
===
--- lldb/source/Host/common/Editline.cpp
+++ lldb/source/Host/common/Editline.cpp
@@ -9,8 +9,9 @@
 #include 
 #include 
 
-#include "lldb/Host/ConnectionFileDescriptor.h"
 #include "lldb/Host/Editline.h"
+
+#include "lldb/Host/ConnectionFileDescriptor.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Utility/CompletionRequest.h"
@@ -641,8 +642,7 @@
   lines.AppendString(new_line_fragment);
 #endif
 
-  int indent_correction = m_fix_indentation_callback(
-  this, lines, 0, m_fix_indentation_callback_baton);
+  int indent_correction = m_fix_indentation_callback(this, lines, 0);
   new_line_fragment = FixIndentation(new_line_fragment, indent_correction);
   m_revert_cursor_index = GetIndentation(new_line_fragment);
 }
@@ -677,8 +677,7 @@
   info->cursor == info->lastchar) {
 if (m_is_input_complete_callback) {
   auto lines = GetInputAsStringList();
-  if (!m_is_input_complete_callback(this, lines,
-m_is_input_complete_callback_baton)) {
+  if (!m_is_input_complete_callback(this, lines)) {
 return BreakLineCommand(ch);
   }
 
@@ -811,8 +810,7 @@
 if (m_fix_indentation_callback) {
   StringList lines = GetInputAsStringList();
   lines.AppendString("");
-  indentation = m_fix_indentation_callback(
-  this, lines, 0, m_fix_indentation_callback_baton);
+  indentation = m_fix_indentation_callback(this, lines, 0);
 }
 m_input_lines.insert(
 m_input_lines.end(),
@@ -857,8 +855,8 @@
   // Save the edits and determine the correct indentation level
   SaveEditedLine();
   StringList lines = GetInputAsStringList(m_current_line_index + 1);
-  int indent_correction = m_fix_indentation_callback(
-  this, lines, cursor_position, m_fix_indentation_callback_baton);
+  int indent_correction =
+  m_fix_indentation_callback(this, lines, cursor_position);
 
   // If it is already correct no special work is needed
   if (indent_correction == 0)
@@ -977,7 +975,7 @@
 }
 
 unsigned char Editline::TabCommand(int ch) {
-  if (m_completion_callback == nullptr)
+  if (!m_completion_callback)
 return CC_ERROR;
 
   const LineInfo *line_info = el_line(m_editline);
@@ -988,7 +986,7 @@
   CompletionResult result;
   CompletionRequest request(line, cursor_index, result);
 
-  m_completion_callback(request, m_completion_callback_baton);
+  m_completion_callback(request);
 
   llvm::ArrayRef results = result.GetResults();
 
@@ -1047,12 +1045,15 @@
 }
 
 unsigned char Editline::ApplyAutosuggestCommand(int ch) {
+  if (!m_suggestion_callback) {
+return CC_REDISPLAY;
+  }
+
   const LineInfo *line_info = el_line(m_editline);
   llvm::StringRef line(line_info->buffer,
line_info->lastcha

[Lldb-commits] [PATCH] D50299: Migrate to llvm::unique_function instead of static member functions for callbacks

[Neal via Phabricator via lldb-commits]

nealsid added a comment.

In D50299#2596141 , @JDevlieghere 
wrote:

> In D50299#2596008 , @nealsid wrote:
>
>> Removed comment on #include line and organized the #includes to match coding 
>> guidelines better (I wasn't sure about whether the project uses blank lines 
>> in between #includes from different subproject like lldb/clang/llvm so I 
>> didn't make the change more broadly)
>
> Looks good. Let me know if you don't have commit access and need me to land 
> this for you.

I do not have commit access, so if you could land it, that would be great.  
Appreciate the review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D50299

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

[Lldb-commits] [PATCH] D98153: Some FormatEntity.cpp cleanup and unit testing

[Neal via Phabricator via lldb-commits]

nealsid updated this revision to Diff 328917.
nealsid added a comment.
Herald added a subscriber: JDevlieghere.

Reuploading patch after I incorrectly set project repo.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98153

Files:
  lldb/include/lldb/Core/FormatEntity.h
  lldb/source/Core/FormatEntity.cpp
  lldb/unittests/Core/CMakeLists.txt
  lldb/unittests/Core/FormatEntityTest.cpp

Index: lldb/unittests/Core/FormatEntityTest.cpp
===
--- /dev/null
+++ lldb/unittests/Core/FormatEntityTest.cpp
@@ -0,0 +1,172 @@
+//===-- FormatEntityTest.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Core/FormatEntity.h"
+#include "lldb/Utility/Status.h"
+
+#include "llvm/ADT/StringRef.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+
+using Definition = FormatEntity::Entry::Definition;
+using Entry = FormatEntity::Entry;
+
+TEST(FormatEntityTest, DefinitionConstructionAllParameters) {
+  Definition d("foo", "bar", FormatEntity::Entry::Type::Invalid, 0, 0, nullptr,
+   false);
+
+  ASSERT_EQ(d.name, "foo");
+  ASSERT_EQ(d.string, "bar");
+  ASSERT_EQ(d.type, FormatEntity::Entry::Type::Invalid);
+  ASSERT_EQ(d.data, 0UL);
+  ASSERT_EQ(d.num_children, 0UL);
+  ASSERT_EQ(d.children, nullptr);
+  ASSERT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameAndType) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid);
+
+  ASSERT_EQ(d.name, "foo");
+  ASSERT_EQ(d.string, nullptr);
+  ASSERT_EQ(d.type, FormatEntity::Entry::Type::Invalid);
+  ASSERT_EQ(d.data, 0UL);
+  ASSERT_EQ(d.num_children, 0UL);
+  ASSERT_EQ(d.children, nullptr);
+  ASSERT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameAndString) {
+  Definition d("foo", "string");
+
+  ASSERT_EQ(d.name, "foo");
+  ASSERT_EQ(d.string, "string");
+  ASSERT_EQ(d.type, FormatEntity::Entry::Type::EscapeCode);
+  ASSERT_EQ(d.data, 0UL);
+  ASSERT_EQ(d.num_children, 0UL);
+  ASSERT_EQ(d.children, nullptr);
+  ASSERT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameTypeData) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid, 33);
+
+  ASSERT_EQ(d.name, "foo");
+  ASSERT_EQ(d.string, nullptr);
+  ASSERT_EQ(d.type, FormatEntity::Entry::Type::Invalid);
+  ASSERT_EQ(d.data, 33UL);
+  ASSERT_EQ(d.num_children, 0UL);
+  ASSERT_EQ(d.children, nullptr);
+  ASSERT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameTypeChildren) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid, 33);
+  Definition parent("parent", FormatEntity::Entry::Type::Invalid, 1, &d);
+  ASSERT_EQ(parent.name, "parent");
+  ASSERT_EQ(parent.string, nullptr);
+  ASSERT_EQ(parent.type, FormatEntity::Entry::Type::Invalid);
+  ASSERT_EQ(parent.num_children, 1UL);
+  ASSERT_EQ(parent.children, &d);
+  ASSERT_FALSE(parent.keep_separator);
+
+  ASSERT_EQ(parent.children[0].name, "foo");
+  ASSERT_EQ(parent.children[0].string, nullptr);
+  ASSERT_EQ(parent.children[0].type, FormatEntity::Entry::Type::Invalid);
+  ASSERT_EQ(parent.children[0].data, 33UL);
+  ASSERT_EQ(parent.children[0].num_children, 0UL);
+  ASSERT_EQ(parent.children[0].children, nullptr);
+  ASSERT_FALSE(d.keep_separator);
+}
+
+constexpr llvm::StringRef lookupStrings[] = {
+"${addr.load}",
+"${addr.file}",
+"${ansi.fg.black}",
+"${ansi.fg.red}",
+"${ansi.fg.green}",
+"${ansi.fg.yellow}",
+"${ansi.fg.blue}",
+"${ansi.fg.purple}",
+"${ansi.fg.cyan}",
+"${ansi.fg.white}",
+"${ansi.bg.black}",
+"${ansi.bg.red}",
+"${ansi.bg.green}",
+"${ansi.bg.yellow}",
+"${ansi.bg.blue}",
+"${ansi.bg.purple}",
+"${ansi.bg.cyan}",
+"${ansi.bg.white}",
+"${file.basename}",
+"${file.dirname}",
+"${file.fullpath}",
+"${frame.index}",
+"${frame.pc}",
+"${frame.fp}",
+"${frame.sp}",
+"${frame.flags}",
+"${frame.no-debug}",
+"${frame.reg.*}",
+"${frame.is-artificial}",
+"${function.id}",
+"${function.name}",
+"${function.name-without-args}",
+"${function.name-with-args}",
+"${function.mangled-name}",
+"${function.addr-offset}",
+"${function.concrete-only-addr-offset-no-padding}",
+"${function.line-offset}",
+"${function.pc-offset}",
+"${function.initial-function}",
+"${function.changed}",
+"${function.is-optimized}",
+"${line.file.basename}",
+"${line.file.dirname}",
+"${line.file.fullpath}",
+"${line.number}",
+"${line.column}",
+"${line.start-addr}",
+"${line.end-

[Lldb-commits] [PATCH] D98153: Some FormatEntity.cpp cleanup and unit testing

[Neal via Phabricator via lldb-commits]

nealsid added a comment.

Thanks - all the comments sound good.  I'll upload a new patch.

It seems like the pre-merge check failures are because I incorrectly set the 
project repo when I first created the revision.  I noticed in the build 
commands here: 
https://buildkite.com/llvm-project/premerge-checks/builds/28649#b755747b-7673-4dbc-9189-2d6bbcd1fbd3
 that the cmake command doesn't include 'lldb' as a project.  I was able to 
repro the error on my Debian machine using those commands, and adding lldb to 
the cmake command seemed to make them go away (tbh, I'm not familiar enough 
with the normal output of clang-tidy to be sure)

Is there any way to fix that without creating a new revision or would that be 
easiest? Thanks, sorry about that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98153

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

[Lldb-commits] [PATCH] D98153: Some FormatEntity.cpp cleanup and unit testing

[Neal via Phabricator via lldb-commits]

nealsid updated this revision to Diff 329266.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98153

Files:
  lldb/include/lldb/Core/FormatEntity.h
  lldb/source/Core/FormatEntity.cpp
  lldb/unittests/Core/CMakeLists.txt
  lldb/unittests/Core/FormatEntityTest.cpp

Index: lldb/unittests/Core/FormatEntityTest.cpp
===
--- /dev/null
+++ lldb/unittests/Core/FormatEntityTest.cpp
@@ -0,0 +1,172 @@
+//===-- FormatEntityTest.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Core/FormatEntity.h"
+#include "lldb/Utility/Status.h"
+
+#include "llvm/ADT/StringRef.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+
+using Definition = FormatEntity::Entry::Definition;
+using Entry = FormatEntity::Entry;
+
+TEST(FormatEntityTest, DefinitionConstructionAllParameters) {
+  Definition d("foo", "bar", FormatEntity::Entry::Type::Invalid, 0, 0, nullptr,
+   false);
+
+  ASSERT_EQ(d.name, "foo");
+  ASSERT_EQ(d.string, "bar");
+  ASSERT_EQ(d.type, FormatEntity::Entry::Type::Invalid);
+  ASSERT_EQ(d.data, 0UL);
+  ASSERT_EQ(d.num_children, 0UL);
+  ASSERT_EQ(d.children, nullptr);
+  ASSERT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameAndType) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid);
+
+  ASSERT_EQ(d.name, "foo");
+  ASSERT_EQ(d.string, nullptr);
+  ASSERT_EQ(d.type, FormatEntity::Entry::Type::Invalid);
+  ASSERT_EQ(d.data, 0UL);
+  ASSERT_EQ(d.num_children, 0UL);
+  ASSERT_EQ(d.children, nullptr);
+  ASSERT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameAndString) {
+  Definition d("foo", "string");
+
+  ASSERT_EQ(d.name, "foo");
+  ASSERT_EQ(d.string, "string");
+  ASSERT_EQ(d.type, FormatEntity::Entry::Type::EscapeCode);
+  ASSERT_EQ(d.data, 0UL);
+  ASSERT_EQ(d.num_children, 0UL);
+  ASSERT_EQ(d.children, nullptr);
+  ASSERT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameTypeData) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid, 33);
+
+  ASSERT_EQ(d.name, "foo");
+  ASSERT_EQ(d.string, nullptr);
+  ASSERT_EQ(d.type, FormatEntity::Entry::Type::Invalid);
+  ASSERT_EQ(d.data, 33UL);
+  ASSERT_EQ(d.num_children, 0UL);
+  ASSERT_EQ(d.children, nullptr);
+  ASSERT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameTypeChildren) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid, 33);
+  Definition parent("parent", FormatEntity::Entry::Type::Invalid, 1, &d);
+  ASSERT_EQ(parent.name, "parent");
+  ASSERT_EQ(parent.string, nullptr);
+  ASSERT_EQ(parent.type, FormatEntity::Entry::Type::Invalid);
+  ASSERT_EQ(parent.num_children, 1UL);
+  ASSERT_EQ(parent.children, &d);
+  ASSERT_FALSE(parent.keep_separator);
+
+  ASSERT_EQ(parent.children[0].name, "foo");
+  ASSERT_EQ(parent.children[0].string, nullptr);
+  ASSERT_EQ(parent.children[0].type, FormatEntity::Entry::Type::Invalid);
+  ASSERT_EQ(parent.children[0].data, 33UL);
+  ASSERT_EQ(parent.children[0].num_children, 0UL);
+  ASSERT_EQ(parent.children[0].children, nullptr);
+  ASSERT_FALSE(d.keep_separator);
+}
+
+constexpr llvm::StringRef lookupStrings[] = {
+"${addr.load}",
+"${addr.file}",
+"${ansi.fg.black}",
+"${ansi.fg.red}",
+"${ansi.fg.green}",
+"${ansi.fg.yellow}",
+"${ansi.fg.blue}",
+"${ansi.fg.purple}",
+"${ansi.fg.cyan}",
+"${ansi.fg.white}",
+"${ansi.bg.black}",
+"${ansi.bg.red}",
+"${ansi.bg.green}",
+"${ansi.bg.yellow}",
+"${ansi.bg.blue}",
+"${ansi.bg.purple}",
+"${ansi.bg.cyan}",
+"${ansi.bg.white}",
+"${file.basename}",
+"${file.dirname}",
+"${file.fullpath}",
+"${frame.index}",
+"${frame.pc}",
+"${frame.fp}",
+"${frame.sp}",
+"${frame.flags}",
+"${frame.no-debug}",
+"${frame.reg.*}",
+"${frame.is-artificial}",
+"${function.id}",
+"${function.name}",
+"${function.name-without-args}",
+"${function.name-with-args}",
+"${function.mangled-name}",
+"${function.addr-offset}",
+"${function.concrete-only-addr-offset-no-padding}",
+"${function.line-offset}",
+"${function.pc-offset}",
+"${function.initial-function}",
+"${function.changed}",
+"${function.is-optimized}",
+"${line.file.basename}",
+"${line.file.dirname}",
+"${line.file.fullpath}",
+"${line.number}",
+"${line.column}",
+"${line.start-addr}",
+"${line.end-addr}",
+"${module.file.basename}",
+"${module.file.dirname}",
+"${module.file.fullpath}",
+"${process.id}",

[Lldb-commits] [PATCH] D98153: Some FormatEntity.cpp cleanup and unit testing

[Neal via Phabricator via lldb-commits]

nealsid marked 3 inline comments as done.
nealsid added inline comments.



Comment at: lldb/include/lldb/Core/FormatEntity.h:144
+num_children(num_children), children(children),
+keep_separator(keep_separator) {}
 };

teemperor wrote:
> I believe you could make this far less verbose with delegating constructors 
> and using C++11 default member initializers.
Makes sense - I used default member initializers but once I did that, 
delegating constructors seemed to add more code back in :) So I just stuck with 
the member initializers.  



Comment at: lldb/source/Core/FormatEntity.cpp:1759
+  return false;
+}
 const char *name = nullptr;

teemperor wrote:
> One-line if's don't have `{}` around the body according to LLVM code style (I 
> know this is wrong in a lot of LLDB code, but newer code should follow that 
> style). Just commenting this once but the same thing is happening above/below 
> this change too.
Done, here and in other spots I noticed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98153

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

[Lldb-commits] [PATCH] D98153: Some FormatEntity.cpp cleanup and unit testing

[Neal via Phabricator via lldb-commits]

nealsid marked 2 inline comments as done.
nealsid added a comment.

In D98153#2610953 , @teemperor wrote:

> Thanks for cleaning this up! Are the `if (!sc) ...` stuff are missing nullptr 
> checks that affect a normal LLDB session (not sure if we can ever have no 
> SymbolContext) or is this just for the unit test?
>
> Anyway, I have some small inline comments but otherwise this looks pretty 
> good to me.

Missed this earlier - I think you're right, because the formatting code is used 
for thread/frame lists & information, so there will be a SymbolContext.  But 
one my planned upcoming changes is to use it in the LLDB prompt, where a target 
might not be available.  And it's also more consistent with formatting code for 
other Entry types.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98153

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

[Lldb-commits] [PATCH] D98153: Some FormatEntity.cpp cleanup and unit testing

[Neal via Phabricator via lldb-commits]

nealsid planned changes to this revision.
nealsid added inline comments.



Comment at: lldb/source/Core/FormatEntity.cpp:1560
 }
-return true;
   }

Sorry, just caught this. Am uploading a new diff shortly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98153

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

[Lldb-commits] [PATCH] D98153: Some FormatEntity.cpp cleanup and unit testing

[Neal via Phabricator via lldb-commits]

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

Fix regression introduced by removing return statement in case where we were 
able to format a function name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98153

Files:
  lldb/include/lldb/Core/FormatEntity.h
  lldb/source/Core/FormatEntity.cpp
  lldb/unittests/Core/CMakeLists.txt
  lldb/unittests/Core/FormatEntityTest.cpp

Index: lldb/unittests/Core/FormatEntityTest.cpp
===
--- /dev/null
+++ lldb/unittests/Core/FormatEntityTest.cpp
@@ -0,0 +1,172 @@
+//===-- FormatEntityTest.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Core/FormatEntity.h"
+#include "lldb/Utility/Status.h"
+
+#include "llvm/ADT/StringRef.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+
+using Definition = FormatEntity::Entry::Definition;
+using Entry = FormatEntity::Entry;
+
+TEST(FormatEntityTest, DefinitionConstructionAllParameters) {
+  Definition d("foo", "bar", FormatEntity::Entry::Type::Invalid, 0, 0, nullptr,
+   false);
+
+  ASSERT_EQ(d.name, "foo");
+  ASSERT_EQ(d.string, "bar");
+  ASSERT_EQ(d.type, FormatEntity::Entry::Type::Invalid);
+  ASSERT_EQ(d.data, 0UL);
+  ASSERT_EQ(d.num_children, 0UL);
+  ASSERT_EQ(d.children, nullptr);
+  ASSERT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameAndType) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid);
+
+  ASSERT_EQ(d.name, "foo");
+  ASSERT_EQ(d.string, nullptr);
+  ASSERT_EQ(d.type, FormatEntity::Entry::Type::Invalid);
+  ASSERT_EQ(d.data, 0UL);
+  ASSERT_EQ(d.num_children, 0UL);
+  ASSERT_EQ(d.children, nullptr);
+  ASSERT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameAndString) {
+  Definition d("foo", "string");
+
+  ASSERT_EQ(d.name, "foo");
+  ASSERT_EQ(d.string, "string");
+  ASSERT_EQ(d.type, FormatEntity::Entry::Type::EscapeCode);
+  ASSERT_EQ(d.data, 0UL);
+  ASSERT_EQ(d.num_children, 0UL);
+  ASSERT_EQ(d.children, nullptr);
+  ASSERT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameTypeData) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid, 33);
+
+  ASSERT_EQ(d.name, "foo");
+  ASSERT_EQ(d.string, nullptr);
+  ASSERT_EQ(d.type, FormatEntity::Entry::Type::Invalid);
+  ASSERT_EQ(d.data, 33UL);
+  ASSERT_EQ(d.num_children, 0UL);
+  ASSERT_EQ(d.children, nullptr);
+  ASSERT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameTypeChildren) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid, 33);
+  Definition parent("parent", FormatEntity::Entry::Type::Invalid, 1, &d);
+  ASSERT_EQ(parent.name, "parent");
+  ASSERT_EQ(parent.string, nullptr);
+  ASSERT_EQ(parent.type, FormatEntity::Entry::Type::Invalid);
+  ASSERT_EQ(parent.num_children, 1UL);
+  ASSERT_EQ(parent.children, &d);
+  ASSERT_FALSE(parent.keep_separator);
+
+  ASSERT_EQ(parent.children[0].name, "foo");
+  ASSERT_EQ(parent.children[0].string, nullptr);
+  ASSERT_EQ(parent.children[0].type, FormatEntity::Entry::Type::Invalid);
+  ASSERT_EQ(parent.children[0].data, 33UL);
+  ASSERT_EQ(parent.children[0].num_children, 0UL);
+  ASSERT_EQ(parent.children[0].children, nullptr);
+  ASSERT_FALSE(d.keep_separator);
+}
+
+constexpr llvm::StringRef lookupStrings[] = {
+"${addr.load}",
+"${addr.file}",
+"${ansi.fg.black}",
+"${ansi.fg.red}",
+"${ansi.fg.green}",
+"${ansi.fg.yellow}",
+"${ansi.fg.blue}",
+"${ansi.fg.purple}",
+"${ansi.fg.cyan}",
+"${ansi.fg.white}",
+"${ansi.bg.black}",
+"${ansi.bg.red}",
+"${ansi.bg.green}",
+"${ansi.bg.yellow}",
+"${ansi.bg.blue}",
+"${ansi.bg.purple}",
+"${ansi.bg.cyan}",
+"${ansi.bg.white}",
+"${file.basename}",
+"${file.dirname}",
+"${file.fullpath}",
+"${frame.index}",
+"${frame.pc}",
+"${frame.fp}",
+"${frame.sp}",
+"${frame.flags}",
+"${frame.no-debug}",
+"${frame.reg.*}",
+"${frame.is-artificial}",
+"${function.id}",
+"${function.name}",
+"${function.name-without-args}",
+"${function.name-with-args}",
+"${function.mangled-name}",
+"${function.addr-offset}",
+"${function.concrete-only-addr-offset-no-padding}",
+"${function.line-offset}",
+"${function.pc-offset}",
+"${function.initial-function}",
+"${function.changed}",
+"${function.is-optimized}",
+"${line.file.basename}",
+"${line.file.dirname}",
+"${line.file.fullpath}",
+"${line.number}",
+"${line.column}",
+"${line.start-addr}",
+   

[Lldb-commits] [PATCH] D98153: Some FormatEntity.cpp cleanup and unit testing

[Neal via Phabricator via lldb-commits]

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

Rebased on HEAD


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98153

Files:
  lldb/include/lldb/Core/FormatEntity.h
  lldb/source/Core/FormatEntity.cpp
  lldb/unittests/Core/CMakeLists.txt
  lldb/unittests/Core/FormatEntityTest.cpp

Index: lldb/unittests/Core/FormatEntityTest.cpp
===
--- /dev/null
+++ lldb/unittests/Core/FormatEntityTest.cpp
@@ -0,0 +1,159 @@
+//===-- FormatEntityTest.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Core/FormatEntity.h"
+#include "lldb/Utility/Status.h"
+
+#include "llvm/ADT/StringRef.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+
+using Definition = FormatEntity::Entry::Definition;
+using Entry = FormatEntity::Entry;
+
+TEST(FormatEntityTest, DefinitionConstructionNameAndType) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid);
+
+  ASSERT_EQ(d.name, "foo");
+  ASSERT_EQ(d.string, nullptr);
+  ASSERT_EQ(d.type, FormatEntity::Entry::Type::Invalid);
+  ASSERT_EQ(d.data, 0UL);
+  ASSERT_EQ(d.num_children, 0UL);
+  ASSERT_EQ(d.children, nullptr);
+  ASSERT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameAndString) {
+  Definition d("foo", "string");
+
+  ASSERT_EQ(d.name, "foo");
+  ASSERT_EQ(d.string, "string");
+  ASSERT_EQ(d.type, FormatEntity::Entry::Type::EscapeCode);
+  ASSERT_EQ(d.data, 0UL);
+  ASSERT_EQ(d.num_children, 0UL);
+  ASSERT_EQ(d.children, nullptr);
+  ASSERT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameTypeData) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid, 33);
+
+  ASSERT_EQ(d.name, "foo");
+  ASSERT_EQ(d.string, nullptr);
+  ASSERT_EQ(d.type, FormatEntity::Entry::Type::Invalid);
+  ASSERT_EQ(d.data, 33UL);
+  ASSERT_EQ(d.num_children, 0UL);
+  ASSERT_EQ(d.children, nullptr);
+  ASSERT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameTypeChildren) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid, 33);
+  Definition parent("parent", FormatEntity::Entry::Type::Invalid, 1, &d);
+  ASSERT_EQ(parent.name, "parent");
+  ASSERT_EQ(parent.string, nullptr);
+  ASSERT_EQ(parent.type, FormatEntity::Entry::Type::Invalid);
+  ASSERT_EQ(parent.num_children, 1UL);
+  ASSERT_EQ(parent.children, &d);
+  ASSERT_FALSE(parent.keep_separator);
+
+  ASSERT_EQ(parent.children[0].name, "foo");
+  ASSERT_EQ(parent.children[0].string, nullptr);
+  ASSERT_EQ(parent.children[0].type, FormatEntity::Entry::Type::Invalid);
+  ASSERT_EQ(parent.children[0].data, 33UL);
+  ASSERT_EQ(parent.children[0].num_children, 0UL);
+  ASSERT_EQ(parent.children[0].children, nullptr);
+  ASSERT_FALSE(d.keep_separator);
+}
+
+constexpr llvm::StringRef lookupStrings[] = {
+"${addr.load}",
+"${addr.file}",
+"${ansi.fg.black}",
+"${ansi.fg.red}",
+"${ansi.fg.green}",
+"${ansi.fg.yellow}",
+"${ansi.fg.blue}",
+"${ansi.fg.purple}",
+"${ansi.fg.cyan}",
+"${ansi.fg.white}",
+"${ansi.bg.black}",
+"${ansi.bg.red}",
+"${ansi.bg.green}",
+"${ansi.bg.yellow}",
+"${ansi.bg.blue}",
+"${ansi.bg.purple}",
+"${ansi.bg.cyan}",
+"${ansi.bg.white}",
+"${file.basename}",
+"${file.dirname}",
+"${file.fullpath}",
+"${frame.index}",
+"${frame.pc}",
+"${frame.fp}",
+"${frame.sp}",
+"${frame.flags}",
+"${frame.no-debug}",
+"${frame.reg.*}",
+"${frame.is-artificial}",
+"${function.id}",
+"${function.name}",
+"${function.name-without-args}",
+"${function.name-with-args}",
+"${function.mangled-name}",
+"${function.addr-offset}",
+"${function.concrete-only-addr-offset-no-padding}",
+"${function.line-offset}",
+"${function.pc-offset}",
+"${function.initial-function}",
+"${function.changed}",
+"${function.is-optimized}",
+"${line.file.basename}",
+"${line.file.dirname}",
+"${line.file.fullpath}",
+"${line.number}",
+"${line.column}",
+"${line.start-addr}",
+"${line.end-addr}",
+"${module.file.basename}",
+"${module.file.dirname}",
+"${module.file.fullpath}",
+"${process.id}",
+"${process.name}",
+"${process.file.basename}",
+"${process.file.dirname}",
+"${process.file.fullpath}",
+"${script.frame}",
+"${script.process}",
+"${script.target}",
+"${script.thread}",
+"${script.var}",
+"${script.svar}",
+"${script.thread}",
+"${svar.dummy-svar-to-test-wildcard}",
+"${thread.id}",
+"${thread.protocol_

[Lldb-commits] [PATCH] D98153: Some FormatEntity.cpp cleanup and unit testing

[Neal via Phabricator via lldb-commits]

nealsid added a comment.

In D98153#2610953 , @teemperor wrote:

> Thanks for cleaning this up! Are the `if (!sc) ...` stuff are missing nullptr 
> checks that affect a normal LLDB session (not sure if we can ever have no 
> SymbolContext) or is this just for the unit test?
>
> Anyway, I have some small inline comments but otherwise this looks pretty 
> good to me.

Thanks again for the review! I should have addressed everything with the new 
diff I just uploaded, but I'm still not sure about the pre-merge check failures 
because it looks (to me) like the build bot should have "lldb" in its cmake 
command, although I will freely admit zero experience with LLVM's build 
infrastructure :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98153

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

[Lldb-commits] [PATCH] D98153: Some FormatEntity.cpp cleanup and unit testing

[Neal via Phabricator via lldb-commits]

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

Thanks for the review - addressed your comments here (I had to rely on Doxygen 
picking up the type reference automatically because I couldn't get the \see 
syntax to work correctly, though)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98153

Files:
  lldb/include/lldb/Core/FormatEntity.h
  lldb/source/Core/FormatEntity.cpp
  lldb/unittests/Core/CMakeLists.txt
  lldb/unittests/Core/FormatEntityTest.cpp

Index: lldb/unittests/Core/FormatEntityTest.cpp
===
--- /dev/null
+++ lldb/unittests/Core/FormatEntityTest.cpp
@@ -0,0 +1,159 @@
+//===-- FormatEntityTest.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Core/FormatEntity.h"
+#include "lldb/Utility/Status.h"
+
+#include "llvm/ADT/StringRef.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+
+using Definition = FormatEntity::Entry::Definition;
+using Entry = FormatEntity::Entry;
+
+TEST(FormatEntityTest, DefinitionConstructionNameAndType) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid);
+
+  EXPECT_EQ(d.name, "foo");
+  EXPECT_EQ(d.string, nullptr);
+  EXPECT_EQ(d.type, FormatEntity::Entry::Type::Invalid);
+  EXPECT_EQ(d.data, 0UL);
+  EXPECT_EQ(d.num_children, 0UL);
+  EXPECT_EQ(d.children, nullptr);
+  EXPECT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameAndString) {
+  Definition d("foo", "string");
+
+  EXPECT_EQ(d.name, "foo");
+  EXPECT_EQ(d.string, "string");
+  EXPECT_EQ(d.type, FormatEntity::Entry::Type::EscapeCode);
+  EXPECT_EQ(d.data, 0UL);
+  EXPECT_EQ(d.num_children, 0UL);
+  EXPECT_EQ(d.children, nullptr);
+  EXPECT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameTypeData) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid, 33);
+
+  EXPECT_EQ(d.name, "foo");
+  EXPECT_EQ(d.string, nullptr);
+  EXPECT_EQ(d.type, FormatEntity::Entry::Type::Invalid);
+  EXPECT_EQ(d.data, 33UL);
+  EXPECT_EQ(d.num_children, 0UL);
+  EXPECT_EQ(d.children, nullptr);
+  EXPECT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameTypeChildren) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid, 33);
+  Definition parent("parent", FormatEntity::Entry::Type::Invalid, 1, &d);
+  EXPECT_EQ(parent.name, "parent");
+  EXPECT_EQ(parent.string, nullptr);
+  EXPECT_EQ(parent.type, FormatEntity::Entry::Type::Invalid);
+  EXPECT_EQ(parent.num_children, 1UL);
+  EXPECT_EQ(parent.children, &d);
+  EXPECT_FALSE(parent.keep_separator);
+
+  EXPECT_EQ(parent.children[0].name, "foo");
+  EXPECT_EQ(parent.children[0].string, nullptr);
+  EXPECT_EQ(parent.children[0].type, FormatEntity::Entry::Type::Invalid);
+  EXPECT_EQ(parent.children[0].data, 33UL);
+  EXPECT_EQ(parent.children[0].num_children, 0UL);
+  EXPECT_EQ(parent.children[0].children, nullptr);
+  EXPECT_FALSE(d.keep_separator);
+}
+
+constexpr llvm::StringRef lookupStrings[] = {
+"${addr.load}",
+"${addr.file}",
+"${ansi.fg.black}",
+"${ansi.fg.red}",
+"${ansi.fg.green}",
+"${ansi.fg.yellow}",
+"${ansi.fg.blue}",
+"${ansi.fg.purple}",
+"${ansi.fg.cyan}",
+"${ansi.fg.white}",
+"${ansi.bg.black}",
+"${ansi.bg.red}",
+"${ansi.bg.green}",
+"${ansi.bg.yellow}",
+"${ansi.bg.blue}",
+"${ansi.bg.purple}",
+"${ansi.bg.cyan}",
+"${ansi.bg.white}",
+"${file.basename}",
+"${file.dirname}",
+"${file.fullpath}",
+"${frame.index}",
+"${frame.pc}",
+"${frame.fp}",
+"${frame.sp}",
+"${frame.flags}",
+"${frame.no-debug}",
+"${frame.reg.*}",
+"${frame.is-artificial}",
+"${function.id}",
+"${function.name}",
+"${function.name-without-args}",
+"${function.name-with-args}",
+"${function.mangled-name}",
+"${function.addr-offset}",
+"${function.concrete-only-addr-offset-no-padding}",
+"${function.line-offset}",
+"${function.pc-offset}",
+"${function.initial-function}",
+"${function.changed}",
+"${function.is-optimized}",
+"${line.file.basename}",
+"${line.file.dirname}",
+"${line.file.fullpath}",
+"${line.number}",
+"${line.column}",
+"${line.start-addr}",
+"${line.end-addr}",
+"${module.file.basename}",
+"${module.file.dirname}",
+"${module.file.fullpath}",
+"${process.id}",
+"${process.name}",
+"${process.file.basename}",
+"${process.file.dirname}",
+"${process.file.fullpath}",
+"${script.frame}",
+"${script.process}",
+"${script.target}",
+"${s

[Lldb-commits] [PATCH] D98153: Some FormatEntity.cpp cleanup and unit testing

[Neal via Phabricator via lldb-commits]

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

CR feedback


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98153

Files:
  lldb/include/lldb/Core/FormatEntity.h
  lldb/source/Core/FormatEntity.cpp
  lldb/unittests/Core/CMakeLists.txt
  lldb/unittests/Core/FormatEntityTest.cpp

Index: lldb/unittests/Core/FormatEntityTest.cpp
===
--- /dev/null
+++ lldb/unittests/Core/FormatEntityTest.cpp
@@ -0,0 +1,159 @@
+//===-- FormatEntityTest.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Core/FormatEntity.h"
+#include "lldb/Utility/Status.h"
+
+#include "llvm/ADT/StringRef.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+
+using Definition = FormatEntity::Entry::Definition;
+using Entry = FormatEntity::Entry;
+
+TEST(FormatEntityTest, DefinitionConstructionNameAndType) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid);
+
+  EXPECT_EQ(d.name, "foo");
+  EXPECT_EQ(d.string, nullptr);
+  EXPECT_EQ(d.type, FormatEntity::Entry::Type::Invalid);
+  EXPECT_EQ(d.data, 0UL);
+  EXPECT_EQ(d.num_children, 0UL);
+  EXPECT_EQ(d.children, nullptr);
+  EXPECT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameAndString) {
+  Definition d("foo", "string");
+
+  EXPECT_EQ(d.name, "foo");
+  EXPECT_EQ(d.string, "string");
+  EXPECT_EQ(d.type, FormatEntity::Entry::Type::EscapeCode);
+  EXPECT_EQ(d.data, 0UL);
+  EXPECT_EQ(d.num_children, 0UL);
+  EXPECT_EQ(d.children, nullptr);
+  EXPECT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameTypeData) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid, 33);
+
+  EXPECT_EQ(d.name, "foo");
+  EXPECT_EQ(d.string, nullptr);
+  EXPECT_EQ(d.type, FormatEntity::Entry::Type::Invalid);
+  EXPECT_EQ(d.data, 33UL);
+  EXPECT_EQ(d.num_children, 0UL);
+  EXPECT_EQ(d.children, nullptr);
+  EXPECT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameTypeChildren) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid, 33);
+  Definition parent("parent", FormatEntity::Entry::Type::Invalid, 1, &d);
+  EXPECT_EQ(parent.name, "parent");
+  EXPECT_EQ(parent.string, nullptr);
+  EXPECT_EQ(parent.type, FormatEntity::Entry::Type::Invalid);
+  EXPECT_EQ(parent.num_children, 1UL);
+  EXPECT_EQ(parent.children, &d);
+  EXPECT_FALSE(parent.keep_separator);
+
+  EXPECT_EQ(parent.children[0].name, "foo");
+  EXPECT_EQ(parent.children[0].string, nullptr);
+  EXPECT_EQ(parent.children[0].type, FormatEntity::Entry::Type::Invalid);
+  EXPECT_EQ(parent.children[0].data, 33UL);
+  EXPECT_EQ(parent.children[0].num_children, 0UL);
+  EXPECT_EQ(parent.children[0].children, nullptr);
+  EXPECT_FALSE(d.keep_separator);
+}
+
+constexpr llvm::StringRef lookupStrings[] = {
+"${addr.load}",
+"${addr.file}",
+"${ansi.fg.black}",
+"${ansi.fg.red}",
+"${ansi.fg.green}",
+"${ansi.fg.yellow}",
+"${ansi.fg.blue}",
+"${ansi.fg.purple}",
+"${ansi.fg.cyan}",
+"${ansi.fg.white}",
+"${ansi.bg.black}",
+"${ansi.bg.red}",
+"${ansi.bg.green}",
+"${ansi.bg.yellow}",
+"${ansi.bg.blue}",
+"${ansi.bg.purple}",
+"${ansi.bg.cyan}",
+"${ansi.bg.white}",
+"${file.basename}",
+"${file.dirname}",
+"${file.fullpath}",
+"${frame.index}",
+"${frame.pc}",
+"${frame.fp}",
+"${frame.sp}",
+"${frame.flags}",
+"${frame.no-debug}",
+"${frame.reg.*}",
+"${frame.is-artificial}",
+"${function.id}",
+"${function.name}",
+"${function.name-without-args}",
+"${function.name-with-args}",
+"${function.mangled-name}",
+"${function.addr-offset}",
+"${function.concrete-only-addr-offset-no-padding}",
+"${function.line-offset}",
+"${function.pc-offset}",
+"${function.initial-function}",
+"${function.changed}",
+"${function.is-optimized}",
+"${line.file.basename}",
+"${line.file.dirname}",
+"${line.file.fullpath}",
+"${line.number}",
+"${line.column}",
+"${line.start-addr}",
+"${line.end-addr}",
+"${module.file.basename}",
+"${module.file.dirname}",
+"${module.file.fullpath}",
+"${process.id}",
+"${process.name}",
+"${process.file.basename}",
+"${process.file.dirname}",
+"${process.file.fullpath}",
+"${script.frame}",
+"${script.process}",
+"${script.target}",
+"${script.thread}",
+"${script.var}",
+"${script.svar}",
+"${script.thread}",
+"${svar.dummy-svar-to-test-wildcard}",
+"${thread.id}",
+"${thread.protocol_id}"

[Lldb-commits] [PATCH] D98153: Some FormatEntity.cpp cleanup and unit testing

[Neal via Phabricator via lldb-commits]

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

Fixed comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98153

Files:
  lldb/include/lldb/Core/FormatEntity.h
  lldb/source/Core/FormatEntity.cpp
  lldb/unittests/Core/CMakeLists.txt
  lldb/unittests/Core/FormatEntityTest.cpp

Index: lldb/unittests/Core/FormatEntityTest.cpp
===
--- /dev/null
+++ lldb/unittests/Core/FormatEntityTest.cpp
@@ -0,0 +1,159 @@
+//===-- FormatEntityTest.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Core/FormatEntity.h"
+#include "lldb/Utility/Status.h"
+
+#include "llvm/ADT/StringRef.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+
+using Definition = FormatEntity::Entry::Definition;
+using Entry = FormatEntity::Entry;
+
+TEST(FormatEntityTest, DefinitionConstructionNameAndType) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid);
+
+  EXPECT_EQ(d.name, "foo");
+  EXPECT_EQ(d.string, nullptr);
+  EXPECT_EQ(d.type, FormatEntity::Entry::Type::Invalid);
+  EXPECT_EQ(d.data, 0UL);
+  EXPECT_EQ(d.num_children, 0UL);
+  EXPECT_EQ(d.children, nullptr);
+  EXPECT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameAndString) {
+  Definition d("foo", "string");
+
+  EXPECT_EQ(d.name, "foo");
+  EXPECT_EQ(d.string, "string");
+  EXPECT_EQ(d.type, FormatEntity::Entry::Type::EscapeCode);
+  EXPECT_EQ(d.data, 0UL);
+  EXPECT_EQ(d.num_children, 0UL);
+  EXPECT_EQ(d.children, nullptr);
+  EXPECT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameTypeData) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid, 33);
+
+  EXPECT_EQ(d.name, "foo");
+  EXPECT_EQ(d.string, nullptr);
+  EXPECT_EQ(d.type, FormatEntity::Entry::Type::Invalid);
+  EXPECT_EQ(d.data, 33UL);
+  EXPECT_EQ(d.num_children, 0UL);
+  EXPECT_EQ(d.children, nullptr);
+  EXPECT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameTypeChildren) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid, 33);
+  Definition parent("parent", FormatEntity::Entry::Type::Invalid, 1, &d);
+  EXPECT_EQ(parent.name, "parent");
+  EXPECT_EQ(parent.string, nullptr);
+  EXPECT_EQ(parent.type, FormatEntity::Entry::Type::Invalid);
+  EXPECT_EQ(parent.num_children, 1UL);
+  EXPECT_EQ(parent.children, &d);
+  EXPECT_FALSE(parent.keep_separator);
+
+  EXPECT_EQ(parent.children[0].name, "foo");
+  EXPECT_EQ(parent.children[0].string, nullptr);
+  EXPECT_EQ(parent.children[0].type, FormatEntity::Entry::Type::Invalid);
+  EXPECT_EQ(parent.children[0].data, 33UL);
+  EXPECT_EQ(parent.children[0].num_children, 0UL);
+  EXPECT_EQ(parent.children[0].children, nullptr);
+  EXPECT_FALSE(d.keep_separator);
+}
+
+constexpr llvm::StringRef lookupStrings[] = {
+"${addr.load}",
+"${addr.file}",
+"${ansi.fg.black}",
+"${ansi.fg.red}",
+"${ansi.fg.green}",
+"${ansi.fg.yellow}",
+"${ansi.fg.blue}",
+"${ansi.fg.purple}",
+"${ansi.fg.cyan}",
+"${ansi.fg.white}",
+"${ansi.bg.black}",
+"${ansi.bg.red}",
+"${ansi.bg.green}",
+"${ansi.bg.yellow}",
+"${ansi.bg.blue}",
+"${ansi.bg.purple}",
+"${ansi.bg.cyan}",
+"${ansi.bg.white}",
+"${file.basename}",
+"${file.dirname}",
+"${file.fullpath}",
+"${frame.index}",
+"${frame.pc}",
+"${frame.fp}",
+"${frame.sp}",
+"${frame.flags}",
+"${frame.no-debug}",
+"${frame.reg.*}",
+"${frame.is-artificial}",
+"${function.id}",
+"${function.name}",
+"${function.name-without-args}",
+"${function.name-with-args}",
+"${function.mangled-name}",
+"${function.addr-offset}",
+"${function.concrete-only-addr-offset-no-padding}",
+"${function.line-offset}",
+"${function.pc-offset}",
+"${function.initial-function}",
+"${function.changed}",
+"${function.is-optimized}",
+"${line.file.basename}",
+"${line.file.dirname}",
+"${line.file.fullpath}",
+"${line.number}",
+"${line.column}",
+"${line.start-addr}",
+"${line.end-addr}",
+"${module.file.basename}",
+"${module.file.dirname}",
+"${module.file.fullpath}",
+"${process.id}",
+"${process.name}",
+"${process.file.basename}",
+"${process.file.dirname}",
+"${process.file.fullpath}",
+"${script.frame}",
+"${script.process}",
+"${script.target}",
+"${script.thread}",
+"${script.var}",
+"${script.svar}",
+"${script.thread}",
+"${svar.dummy-svar-to-test-wildcard}",
+"${thread.id}",
+"${thread.protocol_i

[Lldb-commits] [PATCH] D98153: Some FormatEntity.cpp cleanup and unit testing

[Neal via Phabricator via lldb-commits]

nealsid updated this revision to Diff 337334.
nealsid marked an inline comment as done.
nealsid added a comment.

Update type in for loop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98153

Files:
  lldb/include/lldb/Core/FormatEntity.h
  lldb/source/Core/FormatEntity.cpp
  lldb/unittests/Core/CMakeLists.txt
  lldb/unittests/Core/FormatEntityTest.cpp

Index: lldb/unittests/Core/FormatEntityTest.cpp
===
--- /dev/null
+++ lldb/unittests/Core/FormatEntityTest.cpp
@@ -0,0 +1,159 @@
+//===-- FormatEntityTest.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Core/FormatEntity.h"
+#include "lldb/Utility/Status.h"
+
+#include "llvm/ADT/StringRef.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+
+using Definition = FormatEntity::Entry::Definition;
+using Entry = FormatEntity::Entry;
+
+TEST(FormatEntityTest, DefinitionConstructionNameAndType) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid);
+
+  EXPECT_EQ(d.name, "foo");
+  EXPECT_EQ(d.string, nullptr);
+  EXPECT_EQ(d.type, FormatEntity::Entry::Type::Invalid);
+  EXPECT_EQ(d.data, 0UL);
+  EXPECT_EQ(d.num_children, 0UL);
+  EXPECT_EQ(d.children, nullptr);
+  EXPECT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameAndString) {
+  Definition d("foo", "string");
+
+  EXPECT_EQ(d.name, "foo");
+  EXPECT_EQ(d.string, "string");
+  EXPECT_EQ(d.type, FormatEntity::Entry::Type::EscapeCode);
+  EXPECT_EQ(d.data, 0UL);
+  EXPECT_EQ(d.num_children, 0UL);
+  EXPECT_EQ(d.children, nullptr);
+  EXPECT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameTypeData) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid, 33);
+
+  EXPECT_EQ(d.name, "foo");
+  EXPECT_EQ(d.string, nullptr);
+  EXPECT_EQ(d.type, FormatEntity::Entry::Type::Invalid);
+  EXPECT_EQ(d.data, 33UL);
+  EXPECT_EQ(d.num_children, 0UL);
+  EXPECT_EQ(d.children, nullptr);
+  EXPECT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameTypeChildren) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid, 33);
+  Definition parent("parent", FormatEntity::Entry::Type::Invalid, 1, &d);
+  EXPECT_EQ(parent.name, "parent");
+  EXPECT_EQ(parent.string, nullptr);
+  EXPECT_EQ(parent.type, FormatEntity::Entry::Type::Invalid);
+  EXPECT_EQ(parent.num_children, 1UL);
+  EXPECT_EQ(parent.children, &d);
+  EXPECT_FALSE(parent.keep_separator);
+
+  EXPECT_EQ(parent.children[0].name, "foo");
+  EXPECT_EQ(parent.children[0].string, nullptr);
+  EXPECT_EQ(parent.children[0].type, FormatEntity::Entry::Type::Invalid);
+  EXPECT_EQ(parent.children[0].data, 33UL);
+  EXPECT_EQ(parent.children[0].num_children, 0UL);
+  EXPECT_EQ(parent.children[0].children, nullptr);
+  EXPECT_FALSE(d.keep_separator);
+}
+
+constexpr llvm::StringRef lookupStrings[] = {
+"${addr.load}",
+"${addr.file}",
+"${ansi.fg.black}",
+"${ansi.fg.red}",
+"${ansi.fg.green}",
+"${ansi.fg.yellow}",
+"${ansi.fg.blue}",
+"${ansi.fg.purple}",
+"${ansi.fg.cyan}",
+"${ansi.fg.white}",
+"${ansi.bg.black}",
+"${ansi.bg.red}",
+"${ansi.bg.green}",
+"${ansi.bg.yellow}",
+"${ansi.bg.blue}",
+"${ansi.bg.purple}",
+"${ansi.bg.cyan}",
+"${ansi.bg.white}",
+"${file.basename}",
+"${file.dirname}",
+"${file.fullpath}",
+"${frame.index}",
+"${frame.pc}",
+"${frame.fp}",
+"${frame.sp}",
+"${frame.flags}",
+"${frame.no-debug}",
+"${frame.reg.*}",
+"${frame.is-artificial}",
+"${function.id}",
+"${function.name}",
+"${function.name-without-args}",
+"${function.name-with-args}",
+"${function.mangled-name}",
+"${function.addr-offset}",
+"${function.concrete-only-addr-offset-no-padding}",
+"${function.line-offset}",
+"${function.pc-offset}",
+"${function.initial-function}",
+"${function.changed}",
+"${function.is-optimized}",
+"${line.file.basename}",
+"${line.file.dirname}",
+"${line.file.fullpath}",
+"${line.number}",
+"${line.column}",
+"${line.start-addr}",
+"${line.end-addr}",
+"${module.file.basename}",
+"${module.file.dirname}",
+"${module.file.fullpath}",
+"${process.id}",
+"${process.name}",
+"${process.file.basename}",
+"${process.file.dirname}",
+"${process.file.fullpath}",
+"${script.frame}",
+"${script.process}",
+"${script.target}",
+"${script.thread}",
+"${script.var}",
+"${script.svar}",
+"${script.thread}",
+"${svar.dummy-svar-to-test-wildca

[Lldb-commits] [PATCH] D98153: Some FormatEntity.cpp cleanup and unit testing

[Neal via Phabricator via lldb-commits]

nealsid marked an inline comment as done.
nealsid added inline comments.



Comment at: lldb/unittests/Core/FormatEntityTest.cpp:154
+TEST(FormatEntity, LookupAllEntriesInTree) {
+  for (const auto &testString : lookupStrings) {
+Entry e;

teemperor wrote:
> teemperor wrote:
> > You could just use `llvm::StringRef` here (it's shorter and more 
> > descriptive than the `auto`).
> I actually meant `llvm::StringRef` as the whole type (StringRef references 
> are not really useful as StringRef is already a lightweight String reference)
Done - kept it const since StringRef appears to depend on that for some of its 
methods.  I was looking through StringRef (and the use of optionals for 
something else) and as far as I understand, which could be entirely wrong, it's 
meant to provide a single interface over unowned character data whether it 
comes from a std::string or char*, and thought using string_view would be good, 
as well - is moving to C++17 on the roadmap? I know it's one of those things 
that everyone is in favor of and wants to do but nobody has time for :)



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98153

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

[Lldb-commits] [PATCH] D98153: Some FormatEntity.cpp cleanup and unit testing

[Neal via Phabricator via lldb-commits]

nealsid marked an inline comment as done.
nealsid added a comment.

Thanks for the review, and apologies for the delay in updating the diffs - 
seems like I haven't even had an hour to sit down at my dev machine for the 
past couple weeks, much less actually do any coding :) I don't have commit 
access, so if you could do that, that would be much appreciated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98153

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

[Lldb-commits] [PATCH] D98153: Some FormatEntity.cpp cleanup and unit testing

[Neal via Phabricator via lldb-commits]

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

Small update of patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98153

Files:
  lldb/include/lldb/Core/FormatEntity.h
  lldb/source/Core/FormatEntity.cpp
  lldb/unittests/Core/CMakeLists.txt
  lldb/unittests/Core/FormatEntityTest.cpp

Index: lldb/unittests/Core/FormatEntityTest.cpp
===
--- /dev/null
+++ lldb/unittests/Core/FormatEntityTest.cpp
@@ -0,0 +1,159 @@
+//===-- FormatEntityTest.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Core/FormatEntity.h"
+#include "lldb/Utility/Status.h"
+
+#include "llvm/ADT/StringRef.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+
+using Definition = FormatEntity::Entry::Definition;
+using Entry = FormatEntity::Entry;
+
+TEST(FormatEntityTest, DefinitionConstructionNameAndType) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid);
+
+  EXPECT_EQ(d.name, "foo");
+  EXPECT_EQ(d.string, nullptr);
+  EXPECT_EQ(d.type, FormatEntity::Entry::Type::Invalid);
+  EXPECT_EQ(d.data, 0UL);
+  EXPECT_EQ(d.num_children, 0UL);
+  EXPECT_EQ(d.children, nullptr);
+  EXPECT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameAndString) {
+  Definition d("foo", "string");
+
+  EXPECT_EQ(d.name, "foo");
+  EXPECT_EQ(d.string, "string");
+  EXPECT_EQ(d.type, FormatEntity::Entry::Type::EscapeCode);
+  EXPECT_EQ(d.data, 0UL);
+  EXPECT_EQ(d.num_children, 0UL);
+  EXPECT_EQ(d.children, nullptr);
+  EXPECT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameTypeData) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid, 33);
+
+  EXPECT_EQ(d.name, "foo");
+  EXPECT_EQ(d.string, nullptr);
+  EXPECT_EQ(d.type, FormatEntity::Entry::Type::Invalid);
+  EXPECT_EQ(d.data, 33UL);
+  EXPECT_EQ(d.num_children, 0UL);
+  EXPECT_EQ(d.children, nullptr);
+  EXPECT_FALSE(d.keep_separator);
+}
+
+TEST(FormatEntityTest, DefinitionConstructionNameTypeChildren) {
+  Definition d("foo", FormatEntity::Entry::Type::Invalid, 33);
+  Definition parent("parent", FormatEntity::Entry::Type::Invalid, 1, &d);
+  EXPECT_EQ(parent.name, "parent");
+  EXPECT_EQ(parent.string, nullptr);
+  EXPECT_EQ(parent.type, FormatEntity::Entry::Type::Invalid);
+  EXPECT_EQ(parent.num_children, 1UL);
+  EXPECT_EQ(parent.children, &d);
+  EXPECT_FALSE(parent.keep_separator);
+
+  EXPECT_EQ(parent.children[0].name, "foo");
+  EXPECT_EQ(parent.children[0].string, nullptr);
+  EXPECT_EQ(parent.children[0].type, FormatEntity::Entry::Type::Invalid);
+  EXPECT_EQ(parent.children[0].data, 33UL);
+  EXPECT_EQ(parent.children[0].num_children, 0UL);
+  EXPECT_EQ(parent.children[0].children, nullptr);
+  EXPECT_FALSE(d.keep_separator);
+}
+
+constexpr llvm::StringRef lookupStrings[] = {
+"${addr.load}",
+"${addr.file}",
+"${ansi.fg.black}",
+"${ansi.fg.red}",
+"${ansi.fg.green}",
+"${ansi.fg.yellow}",
+"${ansi.fg.blue}",
+"${ansi.fg.purple}",
+"${ansi.fg.cyan}",
+"${ansi.fg.white}",
+"${ansi.bg.black}",
+"${ansi.bg.red}",
+"${ansi.bg.green}",
+"${ansi.bg.yellow}",
+"${ansi.bg.blue}",
+"${ansi.bg.purple}",
+"${ansi.bg.cyan}",
+"${ansi.bg.white}",
+"${file.basename}",
+"${file.dirname}",
+"${file.fullpath}",
+"${frame.index}",
+"${frame.pc}",
+"${frame.fp}",
+"${frame.sp}",
+"${frame.flags}",
+"${frame.no-debug}",
+"${frame.reg.*}",
+"${frame.is-artificial}",
+"${function.id}",
+"${function.name}",
+"${function.name-without-args}",
+"${function.name-with-args}",
+"${function.mangled-name}",
+"${function.addr-offset}",
+"${function.concrete-only-addr-offset-no-padding}",
+"${function.line-offset}",
+"${function.pc-offset}",
+"${function.initial-function}",
+"${function.changed}",
+"${function.is-optimized}",
+"${line.file.basename}",
+"${line.file.dirname}",
+"${line.file.fullpath}",
+"${line.number}",
+"${line.column}",
+"${line.start-addr}",
+"${line.end-addr}",
+"${module.file.basename}",
+"${module.file.dirname}",
+"${module.file.fullpath}",
+"${process.id}",
+"${process.name}",
+"${process.file.basename}",
+"${process.file.dirname}",
+"${process.file.fullpath}",
+"${script.frame}",
+"${script.process}",
+"${script.target}",
+"${script.thread}",
+"${script.var}",
+"${script.svar}",
+"${script.thread}",
+"${svar.dummy-svar-to-test-wildcard}",
+"${thread.id}",
+"${thread.pr

[Lldb-commits] [PATCH] D101250: Wrap edit line configuration calls into helper functions

[Neal via Phabricator via lldb-commits]

nealsid created this revision.
nealsid requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Currently we call el_set directly to configure the editor in the libedit 
wrapper.  There are some cases in which this causes extra casting, but we pass 
captureless lambdas as function pointers, which should work out of the box.  
Since el_set takes varargs, if the cast is incorrect or if the cast is not 
present, it causes a run time failure rather than compile error.  This change 
makes it so a few different types of configuration is done inside a helper 
function to provide type safety and eliminate that casting.  I didn't do all 
edit line configuration because I'm not sure how important it was in other 
cases and it might require something more general keep up with libedit's 
signature.  I'm open to suggestions, though.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D101250

Files:
  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
@@ -1102,6 +1102,21 @@
   return CC_REDISPLAY;
 }
 
+void Editline::AddFunctionToEditLine(const EditLineCharType *command,
+ const EditLineCharType *helptext,
+ EditlineCommandCallbackType callbackFn) {
+  el_wset(m_editline, EL_ADDFN, command, helptext, callbackFn);
+}
+
+void Editline::SetEditLinePromptCallback(
+EditlinePromptCallbackType callbackFn) {
+  el_set(m_editline, EL_PROMPT, callbackFn);
+}
+
+void Editline::SetGetCharacterFunction(EditlineGetCharCallbackType callbackFn) {
+  el_wset(m_editline, EL_GETCFN, callbackFn);
+}
+
 void Editline::ConfigureEditor(bool multiline) {
   if (m_editline && m_multiline_enabled == multiline)
 return;
@@ -1128,74 +1143,83 @@
   el_set(m_editline, EL_CLIENTDATA, this);
   el_set(m_editline, EL_SIGNAL, 0);
   el_set(m_editline, EL_EDITOR, "emacs");
-  el_set(m_editline, EL_PROMPT,
- (EditlinePromptCallbackType)([](EditLine *editline) {
-   return Editline::InstanceFor(editline)->Prompt();
- }));
 
-  el_wset(m_editline, EL_GETCFN, (EditlineGetCharCallbackType)([](
- EditLine *editline, EditLineGetCharType *c) {
-return Editline::InstanceFor(editline)->GetCharacter(c);
-  }));
+  SetGetCharacterFunction([](EditLine *editline, EditLineGetCharType *c) {
+return Editline::InstanceFor(editline)->GetCharacter(c);
+  });
+
+  SetEditLinePromptCallback([](EditLine *editline) {
+return Editline::InstanceFor(editline)->Prompt();
+  });
 
   // Commands used for multiline support, registered whether or not they're
   // used
-  el_wset(m_editline, EL_ADDFN, EditLineConstString("lldb-break-line"),
-  EditLineConstString("Insert a line break"),
-  (EditlineCommandCallbackType)([](EditLine *editline, int ch) {
-return Editline::InstanceFor(editline)->BreakLineCommand(ch);
-  }));
-  el_wset(m_editline, EL_ADDFN, EditLineConstString("lldb-end-or-add-line"),
-  EditLineConstString("End editing or continue when incomplete"),
-  (EditlineCommandCallbackType)([](EditLine *editline, int ch) {
-return Editline::InstanceFor(editline)->EndOrAddLineCommand(ch);
-  }));
-  el_wset(m_editline, EL_ADDFN, EditLineConstString("lldb-delete-next-char"),
-  EditLineConstString("Delete next character"),
-  (EditlineCommandCallbackType)([](EditLine *editline, int ch) {
-return Editline::InstanceFor(editline)->DeleteNextCharCommand(ch);
-  }));
-  el_wset(
-  m_editline, EL_ADDFN, EditLineConstString("lldb-delete-previous-char"),
+  AddFunctionToEditLine(
+  EditLineConstString("lldb-break-line"),
+  EditLineConstString("Insert a line break"),
+  [](EditLine *editline, int ch) {
+return Editline::InstanceFor(editline)->BreakLineCommand(ch);
+  });
+
+  AddFunctionToEditLine(
+  EditLineConstString("lldb-end-or-add-line"),
+  EditLineConstString("End editing or continue when incomplete"),
+  [](EditLine *editline, int ch) {
+return Editline::InstanceFor(editline)->EndOrAddLineCommand(ch);
+  });
+  AddFunctionToEditLine(
+  EditLineConstString("lldb-delete-next-char"),
+  EditLineConstString("Delete next character"),
+  [](EditLine *editline, int ch) {
+return Editline::InstanceFor(editline)->DeleteNextCharCommand(ch);
+  });
+  AddFunctionToEditLine(
+  EditLineConstString("lldb-delete-previous-char"),
   EditLineConstString("Delete previous character"),
-  (EditlineCommandCallbackType)([](EditLine *editline, int ch) {
+  [](EditLine *editline, int ch) {
 return Editline::InstanceFor(editline)->DeletePreviousCharComm

[Lldb-commits] [PATCH] D101250: Wrap edit line configuration calls into helper functions

[Neal via Phabricator via lldb-commits]

nealsid added a comment.

In D101250#2718107 , @JDevlieghere 
wrote:

> LGTM. I wonder if we would want to wrap this in a macro to get rid of the 
> `EditLineConstString` duplication while keeping the type safety.

Thanks!  I looked into removing the EditLineConstString, but I wasn't a fan of 
having two preprocessor macro expansions.  Maybe it could be a template 
function wchar_t or char.  I also tried some template specialization where the 
functions to call into the edit line library could be instantiated when used, 
but it didn't really appear to save much because all the method signatures for 
edit line parameters have to be manually maintained, and some of them still 
take varargs even after specifying the edit line op parameter., e.g.:

template void editLineCaller();

template<>
void editLineCaller(param1Type param1, param2Type param2) {

  el_set(EL_ADFN, param1, param2);

. . .
}


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101250

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

[Lldb-commits] [PATCH] D101250: Wrap edit line configuration calls into helper functions

[Neal via Phabricator via lldb-commits]

nealsid added a comment.

Friendly ping - let me know if I misunderstood the comments or if anything else 
is needed! Thanks again.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101250

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

[Lldb-commits] [PATCH] D102208: Remove Windows editline from LLDB

[Neal via Phabricator via lldb-commits]

nealsid created this revision.
nealsid added reviewers: LLDB, kparzysz.
Herald added subscribers: hiraditya, krytarowski, mgorny.
nealsid requested review of this revision.
Herald added projects: LLDB, LLVM.
Herald added subscribers: llvm-commits, lldb-commits.

I don't mean to undo others' work but it looks like the hand-rolled EditLine 
for LLDB on Windows isn't used.  It'd be easier to make changes to bring the 
other platforms' Editline wrapper up to date (e.g. simplifying char vs wchar_t) 
without modifying/testing this one too.  I also modified an #ifdef check for 
the MSVC version number to increase it, as it happens on VS2019, too.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D102208

Files:
  lldb/include/lldb/Host/Editline.h
  lldb/include/lldb/Host/windows/editlinewin.h
  lldb/source/Host/CMakeLists.txt
  lldb/source/Host/windows/EditLineWin.cpp
  llvm/lib/Target/Hexagon/HexagonVectorCombine.cpp

Index: llvm/lib/Target/Hexagon/HexagonVectorCombine.cpp
===
--- llvm/lib/Target/Hexagon/HexagonVectorCombine.cpp
+++ llvm/lib/Target/Hexagon/HexagonVectorCombine.cpp
@@ -299,7 +299,7 @@
   return getIfUnordered(dyn_cast(In));
 }
 
-#if !defined(_MSC_VER) || _MSC_VER >= 1924
+#if !defined(_MSC_VER) || _MSC_VER >= 1926
 // VS2017 has trouble compiling this:
 // error C2976: 'std::map': too few template arguments
 template 
Index: lldb/source/Host/windows/EditLineWin.cpp
===
--- lldb/source/Host/windows/EditLineWin.cpp
+++ /dev/null
@@ -1,349 +0,0 @@
-//===-- EditLineWin.cpp ---===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===--===//
-
-// this file is only relevant for Visual C++
-#if defined(_WIN32)
-
-#include "lldb/Host/windows/windows.h"
-
-#include "lldb/Host/windows/editlinewin.h"
-#include "llvm/Support/ErrorHandling.h"
-#include 
-#include 
-
-// edit line EL_ADDFN function pointer type
-typedef unsigned char (*el_addfn_func)(EditLine *e, int ch);
-typedef const char *(*el_prompt_func)(EditLine *);
-
-// edit line wrapper binding container
-struct el_binding {
-  //
-  const char *name;
-  const char *help;
-  // function pointer to callback routine
-  el_addfn_func func;
-  // ascii key this function is bound to
-  const char *key;
-};
-
-// stored key bindings
-static std::vector _bindings;
-
-// TODO: this should in fact be related to the exact edit line context we create
-static void *clientData = NULL;
-
-// store the current prompt string
-// default to what we expect to receive anyway
-static const char *_prompt = "(lldb) ";
-
-#if !defined(_WIP_INPUT_METHOD)
-
-static char *el_get_s(char *buffer, int chars) { return gets_s(buffer, chars); }
-#else
-
-static void con_output(char _in) {
-  HANDLE hout = GetStdHandle(STD_OUTPUT_HANDLE);
-  DWORD written = 0;
-  // get the cursor position
-  CONSOLE_SCREEN_BUFFER_INFO info;
-  GetConsoleScreenBufferInfo(hout, &info);
-  // output this char
-  WriteConsoleOutputCharacterA(hout, &_in, 1, info.dwCursorPosition, &written);
-  // advance cursor position
-  info.dwCursorPosition.X++;
-  SetConsoleCursorPosition(hout, info.dwCursorPosition);
-}
-
-static void con_backspace(void) {
-  HANDLE hout = GetStdHandle(STD_OUTPUT_HANDLE);
-  DWORD written = 0;
-  // get cursor position
-  CONSOLE_SCREEN_BUFFER_INFO info;
-  GetConsoleScreenBufferInfo(hout, &info);
-  // nudge cursor backwards
-  info.dwCursorPosition.X--;
-  SetConsoleCursorPosition(hout, info.dwCursorPosition);
-  // blank out the last character
-  WriteConsoleOutputCharacterA(hout, " ", 1, info.dwCursorPosition, &written);
-}
-
-static void con_return(void) {
-  HANDLE hout = GetStdHandle(STD_OUTPUT_HANDLE);
-  DWORD written = 0;
-  // get cursor position
-  CONSOLE_SCREEN_BUFFER_INFO info;
-  GetConsoleScreenBufferInfo(hout, &info);
-  // move onto the new line
-  info.dwCursorPosition.X = 0;
-  info.dwCursorPosition.Y++;
-  SetConsoleCursorPosition(hout, info.dwCursorPosition);
-}
-
-static bool runBind(char _key) {
-  for (int i = 0; i < _bindings.size(); i++) {
-el_binding *bind = _bindings[i];
-if (bind->key[0] == _key) {
-  bind->func((EditLine *)-1, _key);
-  return true;
-}
-  }
-  return false;
-}
-
-// replacement get_s which is EL_BIND aware
-static char *el_get_s(char *buffer, int chars) {
-  //
-  char *head = buffer;
-  //
-  for (;; Sleep(10)) {
-//
-INPUT_RECORD _record;
-//
-DWORD _read = 0;
-if (ReadConsoleInputA(GetStdHandle(STD_INPUT_HANDLE), &_record, 1,
-  &_read) == FALSE)
-  break;
-// if we didn't read a key
-if (_read == 0)
-  continue;
-// only inter

[Lldb-commits] [PATCH] D102208: Remove Windows editline from LLDB

[Neal via Phabricator via lldb-commits]

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

Updated the diff after moving an unrelated change into another revision.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102208

Files:
  lldb/include/lldb/Host/Editline.h
  lldb/include/lldb/Host/windows/editlinewin.h
  lldb/source/Host/CMakeLists.txt
  lldb/source/Host/windows/EditLineWin.cpp

Index: lldb/source/Host/windows/EditLineWin.cpp
===
--- lldb/source/Host/windows/EditLineWin.cpp
+++ /dev/null
@@ -1,349 +0,0 @@
-//===-- EditLineWin.cpp ---===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===--===//
-
-// this file is only relevant for Visual C++
-#if defined(_WIN32)
-
-#include "lldb/Host/windows/windows.h"
-
-#include "lldb/Host/windows/editlinewin.h"
-#include "llvm/Support/ErrorHandling.h"
-#include 
-#include 
-
-// edit line EL_ADDFN function pointer type
-typedef unsigned char (*el_addfn_func)(EditLine *e, int ch);
-typedef const char *(*el_prompt_func)(EditLine *);
-
-// edit line wrapper binding container
-struct el_binding {
-  //
-  const char *name;
-  const char *help;
-  // function pointer to callback routine
-  el_addfn_func func;
-  // ascii key this function is bound to
-  const char *key;
-};
-
-// stored key bindings
-static std::vector _bindings;
-
-// TODO: this should in fact be related to the exact edit line context we create
-static void *clientData = NULL;
-
-// store the current prompt string
-// default to what we expect to receive anyway
-static const char *_prompt = "(lldb) ";
-
-#if !defined(_WIP_INPUT_METHOD)
-
-static char *el_get_s(char *buffer, int chars) { return gets_s(buffer, chars); }
-#else
-
-static void con_output(char _in) {
-  HANDLE hout = GetStdHandle(STD_OUTPUT_HANDLE);
-  DWORD written = 0;
-  // get the cursor position
-  CONSOLE_SCREEN_BUFFER_INFO info;
-  GetConsoleScreenBufferInfo(hout, &info);
-  // output this char
-  WriteConsoleOutputCharacterA(hout, &_in, 1, info.dwCursorPosition, &written);
-  // advance cursor position
-  info.dwCursorPosition.X++;
-  SetConsoleCursorPosition(hout, info.dwCursorPosition);
-}
-
-static void con_backspace(void) {
-  HANDLE hout = GetStdHandle(STD_OUTPUT_HANDLE);
-  DWORD written = 0;
-  // get cursor position
-  CONSOLE_SCREEN_BUFFER_INFO info;
-  GetConsoleScreenBufferInfo(hout, &info);
-  // nudge cursor backwards
-  info.dwCursorPosition.X--;
-  SetConsoleCursorPosition(hout, info.dwCursorPosition);
-  // blank out the last character
-  WriteConsoleOutputCharacterA(hout, " ", 1, info.dwCursorPosition, &written);
-}
-
-static void con_return(void) {
-  HANDLE hout = GetStdHandle(STD_OUTPUT_HANDLE);
-  DWORD written = 0;
-  // get cursor position
-  CONSOLE_SCREEN_BUFFER_INFO info;
-  GetConsoleScreenBufferInfo(hout, &info);
-  // move onto the new line
-  info.dwCursorPosition.X = 0;
-  info.dwCursorPosition.Y++;
-  SetConsoleCursorPosition(hout, info.dwCursorPosition);
-}
-
-static bool runBind(char _key) {
-  for (int i = 0; i < _bindings.size(); i++) {
-el_binding *bind = _bindings[i];
-if (bind->key[0] == _key) {
-  bind->func((EditLine *)-1, _key);
-  return true;
-}
-  }
-  return false;
-}
-
-// replacement get_s which is EL_BIND aware
-static char *el_get_s(char *buffer, int chars) {
-  //
-  char *head = buffer;
-  //
-  for (;; Sleep(10)) {
-//
-INPUT_RECORD _record;
-//
-DWORD _read = 0;
-if (ReadConsoleInputA(GetStdHandle(STD_INPUT_HANDLE), &_record, 1,
-  &_read) == FALSE)
-  break;
-// if we didn't read a key
-if (_read == 0)
-  continue;
-// only interested in key events
-if (_record.EventType != KEY_EVENT)
-  continue;
-// is the key down
-if (!_record.Event.KeyEvent.bKeyDown)
-  continue;
-// read the ascii key character
-char _key = _record.Event.KeyEvent.uChar.AsciiChar;
-// non ascii conformant key press
-if (_key == 0) {
-  // check the scan code
-  // if VK_UP scroll back through history
-  // if VK_DOWN scroll forward through history
-  continue;
-}
-// try to execute any bind this key may have
-if (runBind(_key))
-  continue;
-// if we read a return key
-if (_key == '\n' || _key == '\r') {
-  con_return();
-  break;
-}
-// key is backspace
-if (_key == 0x8) {
-  // avoid deleting past beginning
-  if (head > buffer) {
-con_backspace();
-head--;
-  }
-  continue;
-}
-
-// add this key to the input buffer
-if ((head - buffer) < (chars - 1)) {
-  con_output(_key

[Lldb-commits] [PATCH] D102208: Remove Windows editline from LLDB

[Neal via Phabricator via lldb-commits]

nealsid added a comment.

I ran LLDB tests from a clean tree and with my change and the failures seemed 
to be the same, but there is a large amount of them.  I also did manual testing 
with the changes.  Build is still green.

Editline is only used when Cmake can find the editline libraries, which it 
never does on Windows.  So the code that is actually used for console I/O is 
the code in the IOHandlerEditline class in IOHandler.cpp that is outside of the 
#ifdef LLDB_USE_EDITLINE preprocessor check.  That uses POSIX functions for 
console I/O.  I think it may be a good follow up change to make 
IOHandlerEditline be used in editline-specific cases and move the 
fallback/non-editline code in that class to another class.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102208

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

[Lldb-commits] [PATCH] D102208: Remove Windows editline from LLDB

[Neal via Phabricator via lldb-commits]

nealsid added a comment.

Thanks, all.  If there are no more comments, could someone land it for me? I 
don't have commit access.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102208

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