[PATCH] D26130: [ELF] - Implemented --symbol-ordering-file option.

[Davide Italiano via llvm-commits]

davide added a comment.

Some comments inline.



================
Comment at: ELF/SymbolListFile.cpp:64-65
+// symbolNameN
+std::vector<StringRef> splitLines(StringRef Lines) {
+  std::vector<StringRef> Ret;
+  while (true) {
----------------
Ehm, splitting a file by line is something that can be probably useful outside of lld, so I propose this to live in ADT/StringRef (or something like that).


================
Comment at: ELF/SymbolListFile.cpp:78-79
+  unsigned I = 0;
+  std::vector<StringRef> Arr = splitLines(MB.getBuffer());
+  for (StringRef S : Arr)
+    Config->SymbolOrderingFile.insert({CachedHashStringRef(S), I++});
----------------
Unless I'm mistaken, you can probably fold `Arr`.


================
Comment at: ELF/Writer.cpp:644
 
+// Sort input sections using list provided by --symbol-ordering-file option.
+template <class ELFT>
----------------
nit: using the list
nit2: s/option//


================
Comment at: ELF/Writer.cpp:646
+template <class ELFT>
+static void sortSectionsByOrder(ArrayRef<OutputSectionBase<ELFT> *> V) {
+  if (Config->SymbolOrderingFile.empty())
----------------
I find the name `sortSectionsByOrder` a little confusing.


================
Comment at: ELF/Writer.cpp:659-660
+      StringRef StrTab = File->getStringTable();
+      StringRef SymName =
+          D->isLocal() ? (StrTab.data() + Body->getNameOffset()) : D->getName();
+
----------------
I prefer details about symbols not being exposed.

So, any reason why you can't use 
```template <class ELFT>
static StringRef getSymbolName(const elf::ObjectFile<ELFT> &File,
                               SymbolBody &Body) {
```
here?


================
Comment at: ELF/Writer.cpp:669-670
+
+  // Sort input sections by order of symbols specified in
+  // --symbol-ordering-file.
+  for (OutputSectionBase<ELFT> *Base : V)
----------------
This just duplicates the comment at the beginning of the function, so I'd either
a) make it more useful
b) remove it altogether


https://reviews.llvm.org/D26130