[PATCH] D17256: [ELF] - Minor refactor of LinkerScript file.

[Rui Ueyama via llvm-commits]

ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.

LGTM with a few nits.


================
Comment at: ELF/LinkerScript.cpp:92
@@ -91,1 +91,3 @@
 private:
+  void init();
+
----------------
Remove this function and move the code inside the constructor.

================
Comment at: ELF/LinkerScript.cpp:143
@@ -124,22 +142,3 @@
     StringRef Tok = next();
-    if (Tok == ";")
-      continue;
-    if (Tok == "ENTRY") {
-      readEntry();
-    } else if (Tok == "EXTERN") {
-      readExtern();
-    } else if (Tok == "GROUP" || Tok == "INPUT") {
-      readGroup();
-    } else if (Tok == "INCLUDE") {
-      readInclude();
-    } else if (Tok == "OUTPUT") {
-      readOutput();
-    } else if (Tok == "OUTPUT_ARCH") {
-      readOutputArch();
-    } else if (Tok == "OUTPUT_FORMAT") {
-      readOutputFormat();
-    } else if (Tok == "SEARCH_DIR") {
-      readSearchDir();
-    } else if (Tok == "SECTIONS") {
-      readSections();
-    } else {
+    auto C = Cmd.find(Tok);
+    if (Cmd.find(Tok) != Cmd.end())
----------------
grimar wrote:
> rafael wrote:
> > rafael wrote:
> > > C is only used inside the if. Move the find call there?
> > > 
> > Actually. The if is on a repeated call to find. So just use 
> > 
> > auto C = Cmd.find(Tok);
> > if (C != Cmd.end())
> >   C->second(*this);
> > 
> > 
> Yep, that what I wanted initially.
Please rename C -> It (because it is not a command but an iterator.)

================
Comment at: ELF/LinkerScript.cpp:144-145
@@ +143,4 @@
+    auto C = Cmd.find(Tok);
+    if (Cmd.find(Tok) != Cmd.end())
+      C->second(*this);
+    else
----------------
Please assign to a local variable to make the actual type explicit.

  if (It != Cmd.end()) {
    std::function<void(ScriptParser &)> &Handler = It->second;
    Handler();
  } else {
    setError(...);
  }


http://reviews.llvm.org/D17256