[PATCH] D14140: [ELF2] SECTIONS command basic support

[Denis Protivensky via llvm-commits]

denis-protivensky added inline comments.

================
Comment at: ELF/Writer.cpp:453
@@ +452,3 @@
+  return true;
+}
+
----------------
grimar wrote:
> This could be in class declaration.
Ok.

================
Comment at: ELF/Writer.cpp:495
@@ -450,4 +494,3 @@
       else
-        static_cast<MergeOutputSection<ELFT> *>(Sec)
-            ->addSection(cast<MergeInputSection<ELFT>>(C));
+        llvm_unreachable("Wrong output section kind");
     }
----------------
grimar wrote:
> Does not seem that llvm_unreachable is really needed. Code just few lines above creates OutputSections or MergeOutputSections, no any other choices.
There are other choices coming from the cached values of `Map`. It now holds only `Bss` before the loop, but who knows what can be added later. That's a good form of protection.

================
Comment at: ELF/Writer.cpp:885
@@ +884,3 @@
+  auto ItEnd = std::end(Config->OutputSections);
+  if (ItA == ItEnd || ItB == ItEnd)
+    return Writer<ELFT>::compareOutputSections(A, B);
----------------
grimar wrote:
> This would work a bit faster in some cases:
> 
> 
> 
> if (Config->OutputSections.find(A->getName()) == ItEnd ||
> Config->OutputSections.find(B->getName()) == ItEnd )
> {...
I don't think we should sacrifice readability since performance benefit is not clear here.
I need iterators after this check, and I don't want to write C-style code like:
```if ((ItA = Config->.... ) == ItEnd ||
  ((ItB = Config->.... ) == ItEnd))```
I will moreover need to define full iterator types before usage instead of `auto` deduction.


http://reviews.llvm.org/D14140