davide requested changes to this revision.
davide added inline comments.
This revision now requires changes to proceed.
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:2079
+ if (log)
+ log->Printf("sorry: unimplemented for XCOFF");
+ return false;
----------------
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 :)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58930/new/
https://reviews.llvm.org/D58930
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
