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

[Meador Inge via Phabricator via llvm-commits]

meadori added inline comments.


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


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


================
Comment at: ELF/LinkerScript.cpp:532
+      return &It->second;
+    error("memory region `" + Cmd->MemoryRegionName + "' not declared");
+    return nullptr;
----------------
ruiu wrote:
> 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.)
Will fix.


================
Comment at: ELF/LinkerScript.cpp:541
+  MemoryRegion *MatchingMemRegion = nullptr;
+  for (auto &MR : Opt.MemoryRegions) {
+    if ((MR.second.Flags & Sec->Flags) != 0 &&
----------------
ruiu wrote:
> We don't usually use `auto` in this context.
This is iterating a map, thus the direct declaration would be something gross like `llvm::DenseMap<llvm::StringRef, MemoryRegion>::value_type`.  Personally, I think the `auto` is cleaner.

Also, there is precedence in the existing code with things like:

```
for (auto I = Cmd->Commands.begin(); I != E; ++I)
    process(**I);
```


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


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


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


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


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


https://reviews.llvm.org/D28911