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