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

[George Rimar via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/SymbolListFile.cpp:64-65
+// symbolNameN
+std::vector<StringRef> splitLines(StringRef Lines) {
+  std::vector<StringRef> Ret;
+  while (true) {
----------------
davide wrote:
> 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).
StringRef::split() can do this job, used it instead.


================
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++});
----------------
davide wrote:
> Unless I'm mistaken, you can probably fold `Arr`.
We don't use regexps in lld anymore. StringMatcher has a list of patterns,
them are unfolded.
So I think keeping SymbolOrderingFile as a DenseMap should be faster.


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


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


================
Comment at: ELF/Writer.cpp:659-660
+      StringRef StrTab = File->getStringTable();
+      StringRef SymName =
+          D->isLocal() ? (StrTab.data() + Body->getNameOffset()) : D->getName();
+
----------------
davide wrote:
> 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?
Done.


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


https://reviews.llvm.org/D26130