[PATCH] D19976: [ELF, WIP] - Prototype of possible linkerscript redesign.

[George Rimar via llvm-commits]

grimar added a comment.

In https://reviews.llvm.org/D19976#485415, @evgeny777 wrote:

> Though, I'm quite new to lld development, I think this code can be simplified. Here is my version of createSections()


Can you post a patch then ? I think it is fine to have few visions and chose the most appropriate. Though my one removes a lot of things, so I belive it is useful change.


================
Comment at: ELF/LinkerScript.cpp:199
@@ -196,4 +198,3 @@
 
-template <class ELFT>
-bool LinkerScript<ELFT>::isDiscarded(InputSectionBase<ELFT> *S) {
-  return getOutputSection(S) == "/DISCARD/";
+size_t LayoutParser::createLayout() {
+  if (!Tokens.size())
----------------
evgeny777 wrote:
> I wonder, if we can live without script parser refactoring for the very first iteration.
> May be this can be done as a separate review?
Not sure here as this patch itself in my understanding a kind of refactoring.

================
Comment at: ELF/LinkerScript.cpp:315
@@ +314,3 @@
+
+    if (Cmd.Kind != InputSectionKind)
+      continue;
----------------
evgeny777 wrote:
> I wonder why do you now mix SectionsRule (which is now InputSectionKind) and SectionsCommand (now OutputSectionKind) into one Command object? Every time you iterate Opt.LayoutParser->Commands, you need either rules or commands, never both. May be it makes sense to have two separate collections, like we had before?
I am not sure what is better. Just afraid that to have 2 collections is a overcomplication itself. Though I`ll think about that.

================
Comment at: ELF/LinkerScript.cpp:350
@@ +349,3 @@
+  auto Assign = [&](StringRef Name) {
+    OutputSectionBase<ELFT> *Sec = findSection<ELFT>(Sections, Name);
+
----------------
evgeny777 wrote:
> You seem to ignore comment in original version of assignAddresses:
> 
> ```
> // Find all the sections with required name. There can be more than
> // ont section with such name, if the alignment, flags or type
> // attribute differs.
> ```
> 
> 
Yes, thanks, this patch uses a bit outdated code. I am going to update it when fix the testcases, though still interested to see general comments.

================
Comment at: ELF/LinkerScript.cpp:386
@@ +385,3 @@
+    if (!Assigned.count(Sec)) {
+      Assigned.insert(Sec);
+      Assign(Sec->getName());
----------------
evgeny777 wrote:
> Do we need to insert to Assigned here? Looks like, we're doing the final processing step
Yes, I think I can remove that, thanks.


https://reviews.llvm.org/D19976