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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/LinkerScript.cpp:419-422
+      error("section '" + CurOutSec->Name + "' will not fit in region '" +
+            CurMemRegion->Name + "'");
+      error("region '" + CurMemRegion->Name + "' overflowed by " +
+            Twine(OverflowAmt) + " bytes");
----------------
You don't want to call `error` twice for one error. You can print out a multi-line error just by adding "\n" in middle of a message.


================
Comment at: ELF/LinkerScript.cpp:541-542
+
+  if (!Opt.MemoryRegions.size())
+    return nullptr;
+
----------------
Do you need this? A `nullptr` will naturally be returned without this, I think.


================
Comment at: ELF/LinkerScript.cpp:549
+    if ((MR.Flags & Sec->Flags) != 0 && (MR.NotFlags & Sec->Flags) == 0) {
+      MatchingMemRegion = &MR;
+      break;
----------------
Return `&MR` here and remove `MatchingMemRegion`


================
Comment at: ELF/LinkerScript.cpp:594
                 [this](std::unique_ptr<BaseCommand> &B) { process(*B.get()); });
+  CurMemRegion = nullptr;
 }
----------------
Why do you have to clear this?


================
Comment at: ELF/LinkerScript.cpp:2047
+      Opt.MemoryRegions[Name] =
+          MemoryRegion(Name, Origin, Length, Flags, NotFlags);
+  }
----------------
You can change this to

  Opt.MemoryRegions[Name] = {Name, Origin, Length, Flags, NotFlags};

can't you?


================
Comment at: ELF/LinkerScript.cpp:2053
+// into a section flag.
+uint32_t ScriptParser::memoryAttributeToFlag(char Attr) {
+  if (tolower(Attr) == 'w')
----------------
You can make this a static non-member function (which is generally preferred because it makes clear that the function does not depend on any class.)


================
Comment at: ELF/LinkerScript.h:196
+struct MemoryRegion {
+  MemoryRegion() : Origin(0), Length(0), Index(0), Flags(0), NotFlags(0) {}
+  MemoryRegion(StringRef N, uint64_t O, uint64_t L, uint32_t F, uint32_t NF)
----------------
Instead of defining this constructor, can you set default values by adding `= 0` to all these fields?


================
Comment at: ELF/LinkerScript.h:197-198
+  MemoryRegion() : Origin(0), Length(0), Index(0), Flags(0), NotFlags(0) {}
+  MemoryRegion(StringRef N, uint64_t O, uint64_t L, uint32_t F, uint32_t NF)
+      : Name(N), Origin(O), Length(L), Index(O), Flags(F), NotFlags(NF) {}
+
----------------
I think you can remove this constructor if you initialize a struct using `{}` as I mentioned below.


https://reviews.llvm.org/D28911