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

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

Few comments.



================
Comment at: ELF/LinkerScript.cpp:743
+      } else {
+        warn("memory region `" + Cmd->MemoryRegionName + "' not declared");
+      }
----------------
Should it be error instead ?


================
Comment at: ELF/LinkerScript.cpp:1988
+    if (consume("(")) {
+      StringRef AttrStr = next();
+      expect(")");
----------------
You do not use AttrStr, so you can just use 'next()'.
May be worth to emit warning/error here about that attributes are not yet supported ?


================
Comment at: ELF/LinkerScript.cpp:2000
+    StringRef Tok = next();
+    if (!readInteger(Tok, Origin))
+      setError("nonconstant expression for origin");
----------------
I think you do not need Tok:

```
if (!readInteger(next(), Origin))
```


================
Comment at: ELF/LinkerScript.cpp:2012
+    // TODO: Fully support constant expressions.
+    if (!readInteger(Tok, Length))
+      setError("nonconstant expression for length");
----------------
Same here.


================
Comment at: ELF/LinkerScript.h:193
+struct MemoryRegion {
+  MemoryRegion() : Name(""), Origin(0), Length(0), Index(0) {}
+  MemoryRegion(StringRef N, uint64_t O, uint64_t L)
----------------
You do not need initialize Name I think, it is empty by default.


https://reviews.llvm.org/D28911