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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
