JDevlieghere added inline comments.
================
Comment at: lldb/source/Utility/Diagnostics.cpp:70
- Error error = Create(FileSpec(diagnostics_dir.str()));
+ Error error = Create(*diagnostics_dir);
if (error) {
----------------
DavidSpickett wrote:
> JDevlieghere wrote:
> > 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.
> Maybe I can be clearer:
> ```
> LLDB diagnostics written to <...>
> Please include the directory content when filing a bug report
> <we crash during writing the files>
> ```
>
> So the files haven't been written, or at least they are in an undefined
> state, but the output looks like we finished writing them. So `will be
> written` is more accurate. If I see this in a test log file I'm going to
> wonder did it crash writing them or did it already write them, which means
> I'd have to reproduce locally. With the sequence of events clearer, I can at
> least check obvious things like a full disk before needing to do that.
>
> Small thing but it's a few words might save some annoyance down the road.
>
> (printing up front I agree with, that part is good)
Makes sense!
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135622/new/
https://reviews.llvm.org/D135622
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
