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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/source/Commands/CommandObjectDiagnostics.cpp:84
+    if (!directory) {
+      result.AppendError(llvm::toString(directory.takeError()));
+      result.SetStatus(eReturnStatusFailed);
----------------
clayborg wrote:
> Do we want to just create a temp directory if the user didn't specify one here and them emit the path to where things were saved instead of erroring out? 
Yes, that's exactly what we do. If we get here we were unable to create either of them.


================
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.">;
+}
----------------
DavidSpickett wrote:
> Worth clarifying whether the path can be absolute or relative to the current working dir?
> 
> I'd guess either works.
Given that either works I think we don't need to be explicit about it.


================
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;
----------------
DavidSpickett wrote:
> You `std::move`'d some errors above does that apply here?
> 
> (I'm not sure exactly what requires it and what doesn't)
`toString` takes ownership of the error, so it needs to be moved here. Errors cannot be copied, so they always need to be moved, except when RVO kicks in. 


================
Comment at: lldb/source/Utility/Diagnostics.cpp:70
 
-  Error error = Create(FileSpec(diagnostics_dir.str()));
+  Error error = Create(*diagnostics_dir);
   if (error) {
----------------
DavidSpickett wrote:
> 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)
Yes, that was @labath's suggestion because when we call this from the signal handler, we might crash again in the process and wouldn't print anything at all. 


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

https://reviews.llvm.org/D135622