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

[Adhemerval Zanella via llvm-commits]

zatrazz added inline comments.

================
Comment at: ELF/DynamicList.cpp:49
@@ +48,3 @@
+  void readEntry(DynamicList::GlobLanguage);
+  DynamicList::GlobLanguage SupportedLanguage(StringRef Lang);
+};
----------------
grimar wrote:
> 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.
In since language demangling is not really supported I will just remove this method.

================
Comment at: ELF/DynamicList.cpp:115
@@ +114,3 @@
+  }
+}
+
----------------
grimar wrote:
> 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 + ")");
> ```
Rui has asked to remove external demangling support, so I will simplify to just:


```
void DynamicListParser::readEntry() {
  DynList->ExactMatch.insert(next());
}
```

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


Repository:
  rL LLVM

http://reviews.llvm.org/D18959