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

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/Config.h:33-35
@@ -31,1 +32,5 @@
 
+struct InputSectionDescription {
+  std::vector<llvm::StringRef> Names;
+};
+
----------------
A struct with one field doesn't have much value. Please remove.

================
Comment at: ELF/LinkerScript.cpp:50-54
@@ -48,5 +49,7 @@
   void readInclude();
+  void readInputSectionDescription(OutputSectionDescription &OutSec);
   void readOutput();
   void readOutputArch();
   void readOutputFormat();
+  void readOutputSectionDescription();
   void readSearchDir();
----------------
Move readInputSectionDescription and readOutputSectionDescription at the end of this list (after one blank line) so that helper functions are not mixed with readers for top-level constructs.

================
Comment at: ELF/LinkerScript.cpp:152-153
@@ +151,4 @@
+    return false;
+
+  // Skip matching token.
+  ++Pos;
----------------
Remove this blank line and a comment. (If you really want to add a comment to this short function, write that as a function comment, but I think we don't need that.)

================
Comment at: ELF/LinkerScript.cpp:245-247
@@ +244,5 @@
+  expect("(");
+  while (!skip(")")) {
+    InSec.Names.push_back(next());
+  }
+}
----------------
You can remove {}.

================
Comment at: ELF/Writer.cpp:435
@@ +434,3 @@
+StringRef Writer<ELFT>::getOutputSectionName(StringRef S) const {
+  if (hasCustomSections()) {
+    auto It = InputToOutputSection.find(S);
----------------
Do you really need to check for that? If it's empty, find() will not find anything, so this check seems extraneous.

================
Comment at: ELF/Writer.cpp:453
@@ +452,3 @@
+template <class ELFT>
+bool Writer<ELFT>::discardInputSection(InputSectionBase<ELFT> *IS) const {
+  if (!IS || !IS->isLive() || IS == &InputSection<ELFT>::Discarded)
----------------
Rename discardInputSection -> isDiscarded

================
Comment at: ELF/Writer.cpp:456
@@ +455,3 @@
+    return true;
+  return hasCustomSections() &&
+         InputToOutputSection.lookup(IS->getSectionName()) == "/DISCARD/";
----------------
Is this check needed?

================
Comment at: ELF/Writer.cpp:463
@@ +462,3 @@
+                                   OutputSectionBase<ELFT> *B) const {
+  if (!hasCustomSections())
+    return compareOutputSections(A, B);
----------------
Is this needed?

================
Comment at: ELF/Writer.cpp:622-626
@@ -574,4 +621,7 @@
 
-  std::stable_sort(OutputSections.begin(), OutputSections.end(),
-                   compareOutputSections<ELFT>);
+  std::stable_sort(
+      OutputSections.begin(), OutputSections.end(),
+      [this](OutputSectionBase<ELFT> *A, OutputSectionBase<ELFT> *B) {
+        return compareSections(A, B);
+      });
 
----------------
This may be able to be written as

  std::stable_sort(OutputSections.begin(), OutputSections.end(), compareSections);



http://reviews.llvm.org/D14140