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

[Sean Fertile via Phabricator via lldb-commits]

sfertile accepted this revision.
sfertile added a comment.

LGTM.



================
Comment at: llvm/lib/MC/MCContext.cpp:165
+    case MCObjectFileInfo::IsXCOFF:
+      // TODO: Need to implement class MCSymbolXCOFF.
+      break;
----------------
jasonliu wrote:
> sfertile wrote:
> > jasonliu wrote:
> > > JDevlieghere wrote:
> > > > See previous comment.
> > > It is certain that we will need MCSymbolXCOFF. But before we run into cases where we actually need a MCSymbolXCOFF, we could use the generic MCSymbol first for XCOFF platform. So I don't want to put a llvm_unreachable here. 
> > Would it make sense to add an llvm_unreachable now, and the first patch that actually uses an MCSymbol stubs out the class and removes the unreachable?
> The first patch that uses MCSymbol do not necessarily need to stub out MCSymbolXCOFF, as MCSymbol seems to be generic and usable until we are doing some XCOFF specific things that needs to be represented by MCSymbolXCOFF. If the intention is MCSymbol should never get used, and different object file should have their own MCSymbolXXX class from the start, then I could add in the llvm_unreachable here, and I would also propose to replace the "return new (Name, *this) MCSymbol(MCSymbol::SymbolKindUnset, Name, IsTemporary);" with an llvm_unreachable as well. 
> 
Ok I think falling through to create the generic MCSymbol in this patch is reasonable. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58930/new/

https://reviews.llvm.org/D58930