[PATCH] D28911: [LinkerScript] Implement `MEMORY` command

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/LinkerScript.cpp:412
 
+  if (CurMemRegion) {
+    CurMemRegion->Index += CurOutSec->Size;
----------------
Comment.


================
Comment at: ELF/LinkerScript.cpp:523
 
+template <class ELFT>
+MemoryRegion *LinkerScript<ELFT>::findMemoryRegion(OutputSectionCommand *Cmd,
----------------
Comment.


================
Comment at: ELF/LinkerScript.cpp:532
+      return &It->second;
+    error("memory region `" + Cmd->MemoryRegionName + "' not declared");
+    return nullptr;
----------------
I don't think we are using `' to quote a word. I believe we write '' instead. (So please make it look similar to other code.)


================
Comment at: ELF/LinkerScript.cpp:541
+  MemoryRegion *MatchingMemRegion = nullptr;
+  for (auto &MR : Opt.MemoryRegions) {
+    if ((MR.second.Flags & Sec->Flags) != 0 &&
----------------
We don't usually use `auto` in this context.


================
Comment at: ELF/LinkerScript.cpp:776-778
+    if (Cmd->AddrExpr) {
       Dot = Cmd->AddrExpr(Dot);
+    }
----------------
Revert this style change.


================
Comment at: ELF/LinkerScript.cpp:2007
+    if (consume("(")) {
+      StringRef AttrStr = next();
+      bool Invert = false;
----------------
Factor out the body of this `if` to a separate function which returns std::pair<uint32_t, uint32_t>.


================
Comment at: ELF/LinkerScript.h:192
 
+struct MemoryRegion {
+  MemoryRegion() : Origin(0), Length(0), Index(0), Flags(0), NotFlags(0) {}
----------------
As I said in the previous comment, this needs a comment.


================
Comment at: ELF/LinkerScript.h:201
+  uint64_t Index;
+  uint32_t Flags, NotFlags;
+};
----------------
We do not write multiple definitions in one one like that. If in doubt, please look at other code and follow the convention.


================
Comment at: ELF/LinkerScript.h:233
+
+  llvm::DenseMap<llvm::StringRef, MemoryRegion> MemoryRegions;
 };
----------------
Needs a comment.


https://reviews.llvm.org/D28911