hubert.reinterpretcast added inline comments.
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:2079
+ if (log)
+ log->Printf("sorry: unimplemented for XCOFF");
+ return false;
----------------
davide wrote:
> hubert.reinterpretcast wrote:
> > JDevlieghere wrote:
> > > jasonliu wrote:
> > > > JDevlieghere wrote:
> > > > > jasonliu wrote:
> > > > > > apaprocki wrote:
> > > > > > > No need to be `sorry:` :) This should probably just say `error:
> > > > > > > XCOFF is unimplemented` to be more direct in case anything is
> > > > > > > expecting "error:" in the output.
> > > > > > Sure. Will address in next revision.
> > > > > Just bundle this with the WASM case, the error message is correct for
> > > > > both.
> > > > I think they are different.
> > > > The error message for WASM seems to suggest that it will never ever get
> > > > supported on WASM.
> > > > But it is not the case for XCOFF, we want to indicate that it is not
> > > > implemented yet.
> > > I don't think the error message suggests that at all, and it's definitely
> > > not true. At this point neither XCOFF nor WASM is supported, and that's
> > > exactly what the log message says.
> > >
> > I agree that the error message for WASM does not indicate that the lack of
> > support is inherent or intended to be permanent; however, it is not
> > indicative either of an intent to implement the support. I am not sure what
> > the intent is for WASM, but I do know that the intent for XCOFF is to
> > eventually implement the support. I do not see how using an ambiguous
> > message in this commit (when we know what the intent is) is superior to the
> > alternative of having an unambiguous message.
> I think we should keep this consistent with the other target so my vote is
> for grouping XCOFF with WASM. After all, if it's going to be implemented
> soon, the message will go away :)
Well, I don't know about "soon"...
Using the WASM message for XCOFF is not actually wrong; so, I can be okay with
it.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58930/new/
https://reviews.llvm.org/D58930
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
