[PATCH] D39511: [ELF] Support expressions with -defsym option

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

I think I like this. Few comments:



================
Comment at: ELF/ScriptLexer.cpp:206
+  if (E == StringRef::npos) {
+    Ret.push_back(S);
+    return Ret;
----------------
What is this code for ? I tried following and no testcases failed for me:

```
  if (E == StringRef::npos) {
    assert(false);
    Ret.push_back(S);
    return Ret;
  }
```


================
Comment at: ELF/ScriptLexer.cpp:229
 // This function may split the current token into multiple tokens.
-void ScriptLexer::maybeSplitExpr() {
-  if (!InExpr || errorCount() || atEOF())
+void ScriptLexer::maybeSplit(std::function<std::vector<StringRef>(StringRef)> Tok) {
+  if (errorCount() || atEOF())
----------------
I think instead of passing function as argument it could probably be better to always call `maybeSplit`
which would do nothing for case when '!InExpr' or '!InDefsym' and it could call either tokenizeDefsym or tokenizeExpr
inside.


================
Comment at: ELF/ScriptLexer.h:41
   bool InExpr = false;
+  bool InDefsym = false;
   size_t Pos = 0;
----------------
Should this be single new enum variable ?


Repository:
  rL LLVM

https://reviews.llvm.org/D39511