[PATCH] D41397: [ELF] - Fix for ld.lld does not accept "AT" syntax for declaring LMA region

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/LinkerScript.cpp:802
+      if (!Sec->LMARegionName.empty())
+        Sec->LMARegion = findMemoryRegion(Sec, Sec->LMARegionName);
+
----------------
This function call is not desirable because we know that `Sec` is not used by the function. Please improve, split or whatever you can do to findMemoryRegion so that its semantics becomes more clear.


================
Comment at: ELF/ScriptParser.cpp:711
 
+  if (consume("AT")) {
+    if (!peek().startswith(">")) {
----------------
This seems unnecessarily complicated. Why don't you just not allow a space between "AT" and ">"?


================
Comment at: ELF/ScriptParser.cpp:714
+      setError("unknown command " + peek());
+    } else if (peek() == ">") {
+      skip();
----------------
Use `consume`



================
Comment at: ELF/ScriptParser.cpp:718
+    } else {
+      Cmd->LMARegionName = next().drop_front();
+    }
----------------
This order isn't easy to understand because it is not obvious what drop_front drops (which is ">", but you need to carefully read the code to find it out.) Please write this way

  if (consume(">"))
  else if (peek().startswith(">"))
  else



https://reviews.llvm.org/D41397