[PATCH] D24604: [ELF] - Implemented --sort-section cmd line option and SORT_NONE script command.

George Rimar via llvm-commits llvm-commits at lists.llvm.org
Thu Sep 15 11:27:21 PDT 2016


grimar added inline comments.

================
Comment at: ELF/LinkerScript.cpp:970-972
@@ -969,3 +969,5 @@
     return SortByAlignment;
+  if (skip("SORT_NONE"))
+    return SortDisabled;
   return SortNone;
 }
----------------
ruiu wrote:
> grimar wrote:
> > ruiu wrote:
> > > This naming scheme is confusing. SORT_NONE is mapped to SortDisabled, and SortNone doesn't mean SORT_NONE?
> > Yes, a bit confusing, but according to documentation SORT_NONE is used to _disable_ sorting. A agree that probably better to map it to SortNone, but what is better name for absence of sorting (which is none) ? SortEmpty probably ?
> I don't know. Null, Empty, Void, Dont, etc... Nothing feels right. I thought about it for a few minutes but didn't come up with a good name. Maybe just return 0?
It is probably a bit strange to return 0 when we have enum. Also problem is that we use the code like that:

```
    if (Cmd->SortInner)
      std::stable_sort(V.begin(), V.end(), getComparator(Cmd->SortInner));
    if (Cmd->SortOuter)
      std::stable_sort(V.begin(), V.end(), getComparator(Cmd->SortOuter));
```

So in that case for this to work we will need to modify start value of enum:

```
enum SortKind { SortDisabled = 1, SortByName, SortByAlignment };
```

What is not very beautiful IMO. I`ll think more about that.


https://reviews.llvm.org/D24604





More information about the llvm-commits mailing list