[PATCH] Linking of shared libraries for MIPS little-endian 32-bit target

Rui Ueyama ruiu at google.com
Thu Dec 12 01:54:06 PST 2013


  LGTM with a few comments. Please run clang-format on your patch before submitting.


================
Comment at: lib/ReaderWriter/ELF/Mips/MipsLinkingContext.cpp:59
@@ +58,3 @@
+public:
+  MipsGOTPassFile(const ELFLinkingContext &eti)
+      : SimpleFile(eti, "MipsGOTPassFile") {
----------------
eti -> ctx?

================
Comment at: lib/ReaderWriter/ELF/Mips/MipsLinkingContext.cpp:124
@@ +123,3 @@
+      break;
+    }
+  }
----------------
You might want to add llvm_unreachable for other cases.

================
Comment at: lib/ReaderWriter/ELF/Mips/MipsLinkingContext.cpp:137
@@ +136,3 @@
+    const DefinedAtom *da = dyn_cast<DefinedAtom>(a);
+    bool isLocal = da && da->scope() == Atom::scopeTranslationUnit;
+
----------------
Add parentheses around the right hand side of the assignment for readabililty.

================
Comment at: lib/ReaderWriter/ELF/Mips/MipsRelocationHandler.cpp:24
@@ +23,3 @@
+  auto target = reinterpret_cast<llvm::support::ulittle32_t *>(location);
+  *target = result | *target;
+}
----------------
Is "|" correct? I don't know about MIPS relocation types but in COFF it's "+".


http://llvm-reviews.chandlerc.com/D2156



More information about the llvm-commits mailing list