[Lldb-commits] [PATCH] D135622: [lldb] Add a "diagnostics dump" command

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.


================
Comment at: lldb/source/Commands/CommandObjectDiagnostics.cpp:85
+      result.AppendError(llvm::toString(directory.takeError()));
+      result.SetStatus(eReturnStatusFailed);
+      return result.Succeeded();
----------------
AppendError sets the status for you now.


================
Comment at: lldb/source/Commands/CommandObjectDiagnostics.cpp:107
+    CommandInterpreter &interpreter)
+    : CommandObjectMultiword(interpreter, "log",
+                             "Commands controlling LLDB diagnostics.",
----------------
Is `"log"` correct here?


================
Comment at: lldb/source/Commands/Options.td:348
+  def diagnostics_dump_directory : Option<"directory", "d">, Group<1>,
+    Arg<"Path">, Desc<"Dump the diagnostics to the given directory.">;
+}
----------------
Worth clarifying whether the path can be absolute or relative to the current working dir?

I'd guess either works.


================
Comment at: lldb/source/Utility/Diagnostics.cpp:62
     stream << "unable to create diagnostic dir: "
-           << toString(errorCodeToError(ec)) << '\n';
+           << toString(diagnostics_dir.takeError()) << '\n';
     return false;
----------------
You `std::move`'d some errors above does that apply here?

(I'm not sure exactly what requires it and what doesn't)


================
Comment at: lldb/source/Utility/Diagnostics.cpp:70
 
-  Error error = Create(FileSpec(diagnostics_dir.str()));
+  Error error = Create(*diagnostics_dir);
   if (error) {
----------------
Silly question, is it intentional that we write out files after saying they have been written?

Of course it makes sense to print something up front, otherwise the error here would come out of the blue, but perhaps "LLDB diagnostics will be written to..." instead?

(this is probably better changed in that earlier patch not sure if that went in yet)


================
Comment at: lldb/test/Shell/Diagnostics/TestDump.test:1
+# Check that the diagnostics dump command uses the correct directory.
+
----------------
"correct directory and creates a new one if needed." ?


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

https://reviews.llvm.org/D135622