[Lldb-commits] [PATCH] D58930: Add XCOFF triple object format type for AIX

[Davide Italiano via Phabricator via lldb-commits]

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