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

Mon Dec 13 10:41:40 PST 2021

labath added inline comments.

================
Comment at: lldb/include/lldb/Host/Editline.h:392-393
+
+  friend class EditlineKeyboardBindingTest_MultiLineEditlineKeybindings_Test;
+  friend class EditlineKeyboardBindingTest_SingleLineEditlineKeybindings_Test;
 };
----------------
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.

================
Comment at: lldb/unittests/Editline/EditlineTest.cpp:86

+  const std::string GetEditlineOutput();
+
----------------
const on a return value is useless

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

================
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.
----------------
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: <https://github.com/llvm/llvm-project/blob/e356027016c6365b3d8924f54c33e2c63d931492/lldb/unittests/Process/Utility/RegisterContextTest.cpp#L20>, <https://github.com/llvm/llvm-project/blob/165545c7a431aa3682fd9468014771c1c5228226/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp#L55>, <https://github.com/llvm/llvm-project/blob/e356027016c6365b3d8924f54c33e2c63d931492/llvm/unittests/Support/DJBTest.cpp#L21>. (Full disclosure: I wrote the last two; while searching for examples, I also found a lot patterns like this which did not have any const annotation whatsoever.)

================
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.
----------------
nealsid wrote:
> 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 :)
Ok, I can accept that.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115126