[PATCH] D29775: [LLD] Add memory ORIGIN and LENGTH expression support

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/LinkerScript.cpp:1873
+      setError("memory region not defined: " + Name);
+    return { [=](uint64_t Dot) { return ScriptConfig->MemoryRegions[Name].Origin; } };
+  }
----------------
Why return has braces?


================
Comment at: ELF/LinkerScript.cpp:1877
+    StringRef Name = readParenLiteral();
+    if( ScriptConfig->MemoryRegions[Name].Name == "" )
+      setError("memory region not defined: " + Name);
----------------
Please format this patch in the LLVM style.


================
Comment at: ELF/LinkerScript.cpp:1871
+    StringRef Name = readParenLiteral();
+    return { [=](uint64_t Dot) { return ScriptConfig->MemoryRegions[Name].Origin; } };
+    }
----------------
ruiu wrote:
> Is this guaranteed that MemoryRegion[Name] exists?
I believe that if a linker script file contains MEMORY commands, they must appear before any SECTIONS commands. Therefore, we can check the existence of MemoryRegions[Name] outside the lambdas. (And raising an error early is better than doing that in the lambdas.)


================
Comment at: test/ELF/linkerscript/symbol-memoryexpr.s:25
+ nop
\ No newline at end of file

----------------
ruiu wrote:
> Please add a newline at end.
You added two newlines -- I wanted to make sure that last line ends with "\n" (which didn't). Now it ends with "\n\n".


https://reviews.llvm.org/D29775