[PATCH] D34781: Introduce a MCReloc class

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

I read through this, but it's hard to see exactly the simplifications that this will yield. I like the follow-up patch to merge PCRel handling between ELF, COFF, and wasm, though.



================
Comment at: include/llvm/MC/MCFixup.h:120
+  SMLoc Loc;
+  uint32_t Offset;
+  const MCSymbolRefExpr *RefA;
----------------
Can you bring along the doxygen comment on MCFixup::Offset? This is typically a small value, it's an offset into the last instruction, not an offset into a fragment or a section.

In general, documenting all these fields would help explain what an MCReloc *is*.


================
Comment at: lib/MC/ELFObjectWriter.cpp:630
                  MCFixupKindInfo::FKF_IsPCRel;
+  MCReloc &Target = Fixup;
+  uint64_t &FixedValue = Fixup.getConstant();
----------------
I take it you plan to rename these things soon, this is just a hack to avoid tons of renames in the diff.

I don't think the deferring of work helps the reviewing, though, because the names don't really make sense right now. An MCReloc is basically a combination of a relocation target and a relocation "location".


https://reviews.llvm.org/D34781