[PATCH] D50494: [LLD] Reserve memory in implicitly set LMARegions

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

Looks good. Few style comments below.



================
Comment at: ELF/LinkerScript.cpp:769
+      return M;
+    }
+  }
----------------
You do not need to wrap `return M` in curly brackets here.


================
Comment at: ELF/LinkerScript.cpp:788
+  // falls into
+  } else if (Sec->LMAExpr) {
+    uint64_t Addr = Sec->LMAExpr().getValue();
----------------
You do not need curly brackets here.
Also, we do not use else after return.
So, it can be something like:

```
if (Sec->LMARegion)
  return Sec->LMARegion;

if (Sec->LMAExpr) {
  uint64_t Addr = Sec->LMAExpr().getValue();
  MemoryRegion *M = findRegion(Addr);
  if (M)
    M->CurPos = Addr;
  return M;
}
```


================
Comment at: ELF/LinkerScript.cpp:807
+    return nullptr;
+  }
+}
----------------
The same.


================
Comment at: ELF/LinkerScript.cpp:813
+    Ctx->MemRegion->CurPos = Addr;
+
+  Dot = Addr;
----------------
Excessive empty line.


================
Comment at: ELF/LinkerScript.cpp:820
 void LinkerScript::assignOffsets(OutputSection *Sec) {
+
+  // First set LMA and Memory regions before modifying any location counter
----------------
I think this empty line is excessive.


================
Comment at: ELF/LinkerScript.cpp:824
+  if (Ctx->LMARegion && !areFlagsCompatible(Ctx->LMARegion, Sec))
+    warn("requested to locate " + Sec->Name + " into lma region with"
+         " incompatible flags");
----------------
I think:
1) LMA perhaps should be uppercase in the error message.
2) Would be nice to include memory region name to this warning.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D50494