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