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

[Meador Inge via Phabricator via llvm-commits]

meadori added inline comments.


================
Comment at: ELF/LinkerScript.cpp:541
+  MemoryRegion *MatchingMemRegion = nullptr;
+  for (auto &MR : Opt.MemoryRegions) {
+    if ((MR.second.Flags & Sec->Flags) != 0 &&
----------------
ruiu wrote:
> meadori wrote:
> > 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);
> > ```
> Got it. Then can you use a temporary variable like this so that the type becomes obvious?
> 
>   MemroyRegion &M = MR.second;
Sure, will do.


================
Comment at: ELF/LinkerScript.cpp:2055
+    uint32_t Flag = 0;
+    switch (C) {
+    case '!':
----------------
grimar wrote:
> I would probably split this into separate function and rewrite to something like:
> 
> ```
> 
> static uint32_t convertToFlags(char Attr) {
>   if (Attr == 'w' || Attr == 'W')
>     return SHF_WRITE;
>   if (Attr == 'x' || Attr == 'X')
>     return SHF_EXECINSTR;
>   if (Attr == 'a' || Attr == 'A')
>     return SHF_ALLOC;
>   if (Attr != 'r' && Attr != 'R')
>     setError("invalid memory region attribute: " + Attr);
> return 0;
> } 
> 
> std::pair<uint32_t, uint32_t> ScriptParser::readMemoryAttributes() {
>   uint32_t Flags = 0;
>   uint32_t NotFlags = 0;
>   bool Invert = false;
>   for (auto C : next()) {
>     if (C == '!)
>       Invert = !Invert;
>     else if (Invert)
>       NotFlags |= convertToFlags(C);
>     else
>       Flags |= convertToFlags(C);
>   }
>   return {Flags, NotFlags};
> }
> 
> ```
I do find that more readable.  Thanks.


================
Comment at: ELF/LinkerScript.cpp:2078
+    }
+    if (Error)
+      break;
----------------
grimar wrote:
> Looks you do not really need that break.
You are right.  Thanks!


https://reviews.llvm.org/D28911