[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;
----------------
zatrazz wrote:
> ruiu wrote:
> > Instead of adding DynamicList, please add DenseSet<StringRef>, because the simple set just works.
> Right, but since the idea is to visit each dynamic list entry to check against the Symtab symbols, why not just a std::vector<llvm::StringRef> as well?
Yeah. Set felt to be a right choice because it is logically a set, but since we are using vector<StringRef> for other things that are logically sets (e.g. Undefined), it probably makes sense more to make this a vector.

================
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;
----------------
zatrazz wrote:
> ruiu wrote:
> > Why did you change this?
> Because the list is already being copies above to be used in the dynamic list processing. Do we really need to get the mapvector again?
I'm sorry but I don't get what you mean. SymtabSymbols is used only once here, so this seems to be just defining a redundant temporary variable and that makes a copy of the mapvector. Am I missing something?


http://reviews.llvm.org/D18959