[PATCH] D16575: [ELF] - Redesign of program headers creation code.

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/Writer.cpp:46
@@ -43,1 +45,3 @@
 private:
+  struct Phdr {
+    Phdr(unsigned Type = 0, unsigned Flags = 0) : Type(Type), Flags(Flags) {}
----------------
This needs comments.

================
Comment at: ELF/Writer.cpp:50
@@ +49,3 @@
+    unsigned Flags;
+    std::list<OutputSectionBase<ELFT> *> OutSections;
+  };
----------------
Please do not use std::list unless you have a reason to do so because it is generally slower than std::vector.

================
Comment at: ELF/Writer.cpp:52
@@ +51,3 @@
+  };
+  struct PhdrsContext {
+    std::vector<std::unique_ptr<Phdr>> PhdrList;
----------------
This needs comments, too. If a purpose of a struct or class is not obvious, please describe.

================
Comment at: ELF/Writer.cpp:58
@@ +57,3 @@
+  };
+  using PhdrMap = PhdrsContext;
+
----------------
What is this for?

================
Comment at: ELF/Writer.cpp:1100
@@ +1099,3 @@
+    std::unique_ptr<Phdr> U = std::make_unique<Phdr>(Type, Flags);
+    return Map.PhdrList.emplace(Map.PhdrList.end(), std::move(U))->get();
+  };
----------------
You are not using a return value.

================
Comment at: ELF/Writer.cpp:1181
@@ +1180,3 @@
+  for (size_t I = 0, E = Map.PhdrList.size(); I != E; ++I) {
+    const auto &PHdr = Map.PhdrList[I];
+    Elf_Phdr *PH = &Phdrs[I];
----------------
Please do not use `auto` unless type is obvious by RHS.

================
Comment at: ELF/Writer.cpp:1201
@@ +1200,3 @@
+
+      for (OutputSectionBase<ELFT> *Sec : PHdr->OutSections) {
+        bool InRelRo = HasRelro && (PHdr->Flags & PF_W) && isRelroSection(Sec);
----------------
Hmm, I'm honestly not sure if this is net positive... This code hasn't changed that much, and this is what I wanted to simplify.


http://reviews.llvm.org/D16575