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