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

[George Rimar via llvm-commits]

grimar added a comment.

Though that patch is different from what I posted, I have no arguments for not to land it for now, until
possible redesign is under discussions. I have just cosmetic сomments about.


================
Comment at: ELF/LinkerScript.cpp:199
@@ -197,3 +198,3 @@
 bool LinkerScript<ELFT>::isDiscarded(InputSectionBase<ELFT> *S) {
-  return getOutputSection(S) == "/DISCARD/";
+  return !S || !S->Live || S->OutSec || getOutputSection(S) == "/DISCARD/";
 }
----------------
This method seems to be a almost copy of elf::isDiscarded(). Do you really need it then ?

================
Comment at: ELF/LinkerScript.cpp:227
@@ +226,3 @@
+LinkerScript<ELFT>::createSections(OutputSectionFactory<ELFT> &Factory) {
+  OutputSectionList<ELFT> Result;
+  typedef std::unique_ptr<elf::ObjectFile<ELFT>> ObjectFileUP;
----------------
I would move Result below typedef, we usually declare typedefs at first lines of method.
Also it's naming is probably incorrect, I believe we do not use 2 uppercases character in a row for naming things. And I think do not leave a type in name (Up == Unique Ptr right ?)

================
Comment at: ELF/LinkerScript.cpp:232
@@ +231,3 @@
+  // output section. Section rules are processed in order they're listed
+  // in script, so correct input section order is maintained by design
+  for (SectionRule &R : Opt.Sections)
----------------
We place dots at the end of comments.

================
Comment at: ELF/LinkerScript.cpp:237
@@ +236,3 @@
+        if (!isDiscarded(S) &&
+            globMatch(R.SectionPattern, S->getSectionName()))
+          // Add single input section to output section
----------------
I would write as

```
if (!isDiscarded(S))
  if (globMatch(R.SectionPattern, S->getSectionName())))
```

================
Comment at: ELF/LinkerScript.cpp:241
@@ +240,3 @@
+
+  // Add all other input sections, which are not listed in script
+  for (const ObjectFileUP &F : Symtab<ELFT>::X->getObjectFiles())
----------------
Ditto about dot.

================
Comment at: ELF/LinkerScript.cpp:243
@@ +242,3 @@
+  for (const ObjectFileUP &F : Symtab<ELFT>::X->getObjectFiles())
+    for (InputSectionBase<ELFT> *C : F->getSections())
+      if (!isDiscarded(C))
----------------
Lets be consistent with code above:

```
InputSectionBase<ELFT> *S
```

================
Comment at: ELF/LinkerScript.h:26
@@ -21,1 +25,3 @@
+template<class ELFT>
+using OutputSectionList = std::vector<std::unique_ptr<OutputSectionBase<ELFT>>>;
 
----------------
I think it is not in common in lld code to declare new type in globals.

================
Comment at: ELF/LinkerScript.h:36
@@ -35,3 +41,2 @@
   StringRef Dest;
-
   StringRef SectionPattern;
----------------
Not relative change.

================
Comment at: ELF/Writer.cpp:92
@@ -91,1 +91,3 @@
+  OutputSectionList<ELFT> OwningSections;
+  OutputSectionFactory<ELFT> Factory;
 
----------------
Looks you can leave Factory to be local object, it does not really needs to be member.
You can just give it to createSections/finalizeSections. I have no real preference here though.


Repository:
  rL LLVM

https://reviews.llvm.org/D22455