[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