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

Tue Dec 14 06:22:59 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;
 };
----------------
nealsid wrote:
> 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.
We have a lot (too many) of friend declarations, but I'm not aware of any case where a production class befriends a test class (particularly one with a gtest-generated name). I know we sometimes make things `protected` (even though they could be `private`) for the sake of testing. That approach would also work here, but I think the new function would be better.

I didn't mean to parse the generated output. The function basically just be a wrapper for `el_set(el, EL_BIND, nullptr)` (I'm assuming that output is suitable for human consumption). The lldb command would then just call this function when invoked.

================
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:
> > 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? 
It was x86_64-linux glibc-2.33 kernel-5.10.

================
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:
> > 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.)
> I understand it may not be done this way elsewhere, but it still captures the intent of the data structure better.  The idea of having a dependency on the declaration to enforce constness, rather than in the definition, doesn't reflect the point of the data structure.
Well, that's the difference. I don't see the constness as a necessary/intrinsic property of the data structure. I just view it as a glorified container -- a nicer version of std::tuple. For all it cares someone may want to dynamically generate test cases. It's up to the user to say what he wants to do with it. And the user -- the actual variable -- wants to provide a compile time list of test cases, and there it's perfectly natural to be const.

And in general, it's very rare to see const on a data member in c++, so upon seeing that, my mind automatically goes searching for some subtle reason for why it has to be there -- and then gets confused when it doesn't find any. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115126