[PATCH] D34781: Introduce a MCReloc class

[Gerolf Hoflehner via Phabricator via llvm-commits]

Gerolf added reviewers: ab, Gerolf.
Gerolf added a comment.

Just a first superficial review. I haven't thought about the underlying concept itself yet.

-Gerolf



================
Comment at: include/llvm/MC/MCFixup.h:156
+
+  const MCExpr *getValue() const { return Expr; }
+  void setValue(const MCExpr* Val) { Expr = Val; }
----------------
getExpr, setExpr?


================
Comment at: include/llvm/MC/MCFixup.h:159
+
+  bool isAbsolute() const { return !RefA && !SymB; }
+  MCSymbolRefExpr::VariantKind getAccessVariant() const {
----------------
Is there a better name? Absolute what?


================
Comment at: lib/MC/ELFObjectWriter.cpp:633
   const MCSectionELF &FixupSection = cast<MCSectionELF>(*Fragment->getParent());
   uint64_t C = Target.getConstant();
   uint64_t FixupOffset = Layout.getFragmentOffset(Fragment) + Fixup.getOffset();
----------------
Same as FixedValue?


================
Comment at: lib/MC/MCAssembler.cpp:202
+  uint64_t &Value = Reloc.getConstant();
   Value = 0;
   if (!Expr->evaluateAsRelocatable(Target, &Layout, &Fixup)) {
----------------
Reloc.setConstant(0)?


================
Comment at: lib/MC/MCAssembler.cpp:249
 
+  // Let the backend check whether we need a relocation.
+  Backend.processFixupValue(*this, Fixup, Target, IsResolved);
----------------
The comments are at least confusing. From the check below it can be derived that IsResolved has the information where or not a relocation is needed already. Then there is no need to check for it.


https://reviews.llvm.org/D34781