[PATCH] D22455: [ELF] Create output sections in LinkerScript class

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/LinkerScript.cpp:198
@@ -196,2 +197,3 @@
 template <class ELFT>
 bool LinkerScript<ELFT>::isDiscarded(InputSectionBase<ELFT> *S) {
+  return !S || !S->Live || S->OutSec || getOutputSection(S) == "/DISCARD/";
----------------
Having two functions with the same name (elf::isDicarded and LinkerScript::isDiscarded) is confusing because from LinkerScript class, the latter hides the former.

Can you move elf::isDiscarded to Writer.cpp and make it a static non-member function? Then the other isDiscarded is not visible from this file, so it is less confusing.

================
Comment at: ELF/LinkerScript.cpp:211
@@ -209,1 +210,3 @@
 template <class ELFT>
+static void addInputSection(
+    OutputSectionFactory<ELFT> &Factory, InputSectionBase<ELFT> *C,
----------------
This is a helper function for createSections and heavily relies on it. I usually write such function as a lambda for readability -- such as `Add` in OutputSections.cpp.

================
Comment at: ELF/LinkerScript.cpp:228
@@ +227,3 @@
+  OutputSectionList<ELFT> Result;
+  typedef std::unique_ptr<elf::ObjectFile<ELFT>> ObjectFileUP;
+
----------------
For consistency, please remove this typedef.

================
Comment at: ELF/LinkerScript.cpp:245
@@ +244,3 @@
+      if (!isDiscarded(C))
+        addInputSection(Factory, C, getOutputSectionName(C), Result);
+
----------------
Does this work? It adds the same section again if it is mentioned in a linker script.

================
Comment at: ELF/LinkerScript.h:26
@@ -21,1 +25,3 @@
+template<class ELFT>
+using OutputSectionList = std::vector<std::unique_ptr<OutputSectionBase<ELFT>>>;
 
----------------
grimar wrote:
> I think it is not in common in lld code to declare new type in globals.
Yup. We don't usually do this, so it is better to avoid it for consistency.

================
Comment at: ELF/Writer.cpp:613
@@ -605,7 +612,3 @@
 
-// Create output section objects and add them to OutputSections.
 template <class ELFT> void Writer<ELFT>::createSections() {
   for (const std::unique_ptr<elf::ObjectFile<ELFT>> &F :
----------------
Can you make this function return a vector of sections rather than mutating OwningSections? Then, I think you can remove OwningSections member from this class by making it a local variable in `run`.


Repository:
  rL LLVM

https://reviews.llvm.org/D22455