[PATCH] D30500: [ELF] - Do not treat colon(":") as separate token in script parser.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/LinkerScript.cpp:1945
+    // "global:" is default, so if there's no label, we assume global symbols.
+    consumeLabel("global:");
+    for (SymbolVersion V : readSymbols(false))
----------------
ruiu wrote:
> `:` is not a part of a label. It's a punctuation after a label. So please remove it.
Done.


================
Comment at: ELF/LinkerScript.cpp:2004-2009
+    if (consumeLabel("local:")) {
+      if (Locals)
+        setError("nested local label");
+      readLocals();
+      return Ret;
+    }
----------------
ruiu wrote:
> Do you need this error check?
Otherwise we would accept scripts like:

```
{
global:
...
local:
...
local:
...
local:
...
}
```

Both bfd and gold does not accept this, I think we also should not.

Code would be really simpler if we had peekLabel() I believe. Still not understand why we can't have one. consumeLabel can me implemented via peekLabel() + next(), so at fact we would have single method relying on LL(2) in final anyways.


================
Comment at: ELF/ScriptLexer.cpp:252
+bool ScriptLexer::consumeLabel(StringRef Tok) {
+  if (peek() == Tok ||
+    (peek() == Tok.drop_back(1) && peek(1) == Tok.take_back(1))) {
----------------
ruiu wrote:
> This seems complicated. You want to skip either `Tok` or `Tok:`. In code, it's something like this.
> 
>   if (consume(Tok))
>     expect(":");
>   else
>     expect(Tok + ":");
No, that is not like this. I can't consume Tok and expect(":") for example because Tok may be a "local" symbol name, and not a label. I can't expect anything in this method actually.

expect(Tok + ":"); sure simplifies things, but requires using Twine argument (what involves std::string creation for each consume call, hope that is fine).


================
Comment at: ELF/ScriptLexer.cpp:260-263
+void ScriptLexer::skip(unsigned N) {
+  while (N--)
+    (void)next();
+}
----------------
ruiu wrote:
> Please do not introduce a new utility function. If you want to skip two tokens, call next() twice.
OK.


https://reviews.llvm.org/D30500