[PATCH] D19716: [mips] Use MipsMCExpr instead of MCSymbolRefExpr for all relocations.

[Simon Dardis via llvm-commits]

sdardis added a comment.

This mostly looks LGTM but I have some inlined comments.


================
Comment at: lib/Target/Mips/InstPrinter/MipsInstPrinter.cpp:153
@@ -181,6 +152,3 @@
 
-  if ((Kind == MCSymbolRefExpr::VK_Mips_GPOFF_HI) ||
-      (Kind == MCSymbolRefExpr::VK_Mips_GPOFF_LO))
-    OS << ")))";
-  else if (Kind != MCSymbolRefExpr::VK_None)
+  if (Kind != MCSymbolRefExpr::VK_None)
     OS << ')';
----------------
Am I missing something here? As far as I can see, you assert that Kind == VK_None on line 143 but here you're testing if Kind != VK_None, which I believe is always false.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp:467
@@ -463,1 +466,3 @@
+  // The caller will also ignore any changes we make to Value
+  // (recordRelocation() overwrite it with it's own calculation).
   (void)adjustFixupValue(Fixup, Value, &Asm.getContext());
----------------
Nit: overwrite -> overwrites

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsMCExpr.h:85-86
@@ +84,4 @@
+  bool isGpOff() const {
+    MipsExprKind Kind;
+    return isGpOff(Kind);
+  }
----------------
Should this be 'return isGpOff(Kind);' as the local declaration of 'Kind' here shadows the class definition.


http://reviews.llvm.org/D19716