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

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/DynamicList.cpp:49
@@ +48,3 @@
+  void readEntry(DynamicList::GlobLanguage);
+  DynamicList::GlobLanguage SupportedLanguage(StringRef Lang);
+};
----------------
Its a bit strange function name, there is standart (http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly) saying that "Function names should be verb phrases (as they represent actions)"
So I would add "get":
```
getSupportedLanguage(StringRef Lang);
```
Probably "get" is not best word here, but it shows the idea. And also function name is uppercase, but should be lowercase.

================
Comment at: ELF/DynamicList.cpp:115
@@ +114,3 @@
+  }
+}
+
----------------
Looks this can be simplified:
```
StringRef Entry = next();
if (Entry != "extern" || peek() == ";") {
  DynList->ExactMatch.insert(Entry);
  return;
}

StringRef Tok = next();
DynamicList::GlobLanguage Lang = SupportedLanguage(Tok);
if (Lang != DynamicList::LANGUAGE_NOT_SUPPORTED)
  readLanguageGroup(Lang);
else
  setError("invalid language in extern definition (" + Tok + ")");
```

================
Comment at: ELF/DynamicList.cpp:120
@@ +119,3 @@
+    readGroup();
+  }
+}
----------------
You can remove brackets.
```
  while (!atEOF())
    readGroup();
```

================
Comment at: ELF/DynamicList.cpp:130
@@ +129,3 @@
+    return true;
+  return false;
+}
----------------
I would write:

```
return ExactMatch.find(Symbol) != ExactMatch.end();
```

================
Comment at: ELF/Options.td:169
@@ -168,1 +168,3 @@
 
+def dynamic_list : Separate<["--", "-"], "dynamic-list">,
+  HelpText<"Dynamic list to use">;
----------------
Not sorted.


Repository:
  rL LLVM

http://reviews.llvm.org/D18959