[PATCH] D24685: Simplify SORT and --sort-section command line option handling.

[George Rimar via llvm-commits]

grimar added a comment.

My comments below, every of them is just an IMHO of course.


================
Comment at: ELF/LinkerScript.cpp:169
@@ +168,3 @@
+    std::stable_sort(Sections.begin(), Sections.end(), getComparator(K));
+}
+
----------------
So you can call sortSections() or not call it, and function itself can
do sort or not, while it's name contains "sort" explicitly.

I understand why you had to do all that, but it is much 
more confusing than was for me.


================
Comment at: ELF/LinkerScript.cpp:189
@@ +188,3 @@
+  // Sort sections by --sort-section and SORT_*() commands.
+  // We do not sort it at all if it was surrounded by SORT_NONE().
+  if (I->SortOuter != SortSectionPolicy::None) {
----------------
This comment says nothing about how rules are applied, 
selectSortKind() had many of them, though I think it could leave fully without,
as SortSectionPolicy had very clear constant names.

I can't say the same for this code though.
I thought you wanted to have more comments generally,
if they helps to avoid reading code too much.
Do you find them excessive in your variant ?

================
Comment at: ELF/LinkerScript.cpp:1012
@@ -1011,3 @@
-    Cmd->SortOuter = SortSectionPolicy::Default;
-    return;
-  }
----------------
By the way this place did not need the comment before renaming IgnoreConfig to None, I think code on the left side (if take version before renaming) is much more readable without need of in-mind-debugging.


https://reviews.llvm.org/D24685