[PATCH] D24758: [ELF] - Linkerscript: support complex section pattern grammar.
George Rimar via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 21 04:40:09 PDT 2016
grimar added inline comments.
================
Comment at: ELF/LinkerScript.cpp:159-160
@@ -158,1 +158,4 @@
+static void sortSections(std::vector<InputSectionData *>::iterator S,
+ std::vector<InputSectionData *>::iterator E,
+ SortSectionPolicy K) {
----------------
ruiu wrote:
> Can you pass two pointers to InputSectionData instead of iterators?
>
> S and E? Can you rename them Begin and End if they mean begin and end?
It was Start/End. And it is actually pointers to pointers here, I am not sure it looks better than iterators, but done.
================
Comment at: ELF/LinkerScript.cpp:197-198
@@ +196,4 @@
+ // 4. If no SORT command is given, sort according to --sort-section.
+ auto ItS = I->Sections.begin() + SizeBefore;
+ auto ItE = I->Sections.end();
+ if (Pat.SortOuter != SortSectionPolicy::None) {
----------------
ruiu wrote:
> InputSectionData *Begin = I->Sections.data() + SizeBefore;
> InputSectionData *End = I->Sections.data() + I->Sections.size();
Done.
================
Comment at: ELF/LinkerScript.cpp:764-765
@@ -741,2 +763,4 @@
Regex readFilePatterns();
- void readSectionExcludes(InputSectionDescription *Cmd);
+ void readInputSectionsList(std::vector<SectionPattern> &Out,
+ SortSectionPolicy K1, SortSectionPolicy K2);
+ bool readSortedInputSectionsList(std::vector<SectionPattern> &Out);
----------------
ruiu wrote:
> Instead of mutating an argument, create a new vector and return it. It is the common practice in this file.
Done.
================
Comment at: ELF/LinkerScript.cpp:1098-1099
@@ -1092,1 +1097,4 @@
+bool ScriptParser::readSortedInputSectionsList(
+ std::vector<SectionPattern> &Out) {
+ SortSectionPolicy SortOut = readSortKind();
----------------
ruiu wrote:
> Returns a new vector instead of mutating the argument.
Done.
================
Comment at: ELF/LinkerScript.h:104-105
@@ -102,3 +103,4 @@
struct SectionPattern {
- SectionPattern(llvm::Regex &&Re1, llvm::Regex &&Re2)
+ SectionPattern(llvm::Regex &&Re1, llvm::Regex &&Re2, SortSectionPolicy So1,
+ SortSectionPolicy So2)
: ExcludedFileRe(std::forward<llvm::Regex>(Re1)),
----------------
ruiu wrote:
> So1 -> Sort1
>
> RE is a common acronym for Regular Expression, but SO is not.
Done.
================
Comment at: ELF/LinkerScript.h:119-120
@@ -113,2 +118,4 @@
llvm::Regex SectionRe;
+ SortSectionPolicy SortOuter = SortSectionPolicy::Default;
+ SortSectionPolicy SortInner = SortSectionPolicy::Default;
};
----------------
ruiu wrote:
> Remove `= ... Default`. You seem to always assign them.
Done.
https://reviews.llvm.org/D24758
More information about the llvm-commits
mailing list