[Lldb-commits] [PATCH] D135631: [lldb] Copy log files into diagnostic directory

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/source/Core/Debugger.cpp:815
+    Diagnostics::Instance().AddCallback(
+        [&](const FileSpec &dir) -> llvm::Error {
+          for (auto &entry : m_stream_handlers) {
----------------
A default reference capture is dangerous when the lambda is supposed to run after the enclosing function returns. I guess you really wanted to capture `this` (?)
Also, I would expect that means that you need to disable the callback in the Debugger destructor.


================
Comment at: lldb/test/Shell/Diagnostics/TestCopyLogs.test:3
+# RUN: mkdir -p %t
+# The "ll''db" below is not a typo but a way to prevent lit from substituting 'lldb'.
+# RUN: %lldb -o 'log enable ll''db commands -f %t/commands.log' -o 'diagnostics dump -d %t/diags'
----------------
JDevlieghere wrote:
> labath wrote:
> > That's cute, but I suspect windows will have a problem with that. `-s %s` (and putting the commands in this file) would be safer.
> I thought about that, but I still need `%t` to be expanded. I guess I could put `log enable lldb commands -f ` in a file and append `%t/commands.log` to it. 
hm.. the %t substitution makes it tricky, but I don't think this is better than what we had before. It still has nested quotes, which is the thing that causes problems on windows. I think you could work around this by creating an alias in a command file (say `command alias logcommands log enable lldb commands`) and then invoking it from the command line (`-o "logcommands -f %t/commands.log"`).


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

https://reviews.llvm.org/D135631