[PATCH] D18959: [lld] Implement --dynamic-list

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/Config.h:54
@@ -52,2 +53,3 @@
   std::vector<llvm::StringRef> Undefined;
+  DynamicList DynList;
   bool AllowMultipleDefinition;
----------------
Instead of adding DynamicList, please add DenseSet<StringRef>, because the simple set just works.

================
Comment at: ELF/Driver.cpp:135
@@ -138,1 +134,3 @@
 
+ErrorOr<MemoryBufferRef> LinkerDriver::readFile(StringRef Path) {
+  using namespace llvm::sys::fs;
----------------
Since we handle errors inside this function, it is rather confusing to propagate a detailed error back to callers. I'd use llvm::Optional instead of ErrorOr.

================
Comment at: ELF/Driver.cpp:136
@@ +135,3 @@
+ErrorOr<MemoryBufferRef> LinkerDriver::readFile(StringRef Path) {
+  using namespace llvm::sys::fs;
+  auto MBOrErr = MemoryBuffer::getFile(Path);
----------------
Do you need this?

================
Comment at: ELF/Writer.cpp:1115
@@ +1114,3 @@
+      Symtab.getSymbols();
+  // Process the --dynamic-list options.
+  for (StringRef S : Config->DynList.getSymbols()) {
----------------
This comment needs to be more explanatory.

  // The dyanmic list is a feature to give a list of symbols that need
  // to be exported via the dynamic symbol table. Here is where the
  // dynamic list is processed.

================
Comment at: ELF/Writer.cpp:1124
@@ -1114,3 +1123,3 @@
   std::vector<DefinedCommon *> CommonSymbols;
-  for (auto &P : Symtab.getSymbols()) {
+  for (auto &P : SymtabSymbols) {
     SymbolBody *Body = P.second->Body;
----------------
Why did you change this?


http://reviews.llvm.org/D18959