[PATCH] D22455: [ELF] Create output sections in LinkerScript class
George Rimar via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 18 04:56:36 PDT 2016
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
More information about the llvm-commits
mailing list