[PATCH] [lld] add InputGraph to process Command line arguments

Michael Spencer bigcheesegs at gmail.com
Tue Jul 30 14:56:44 PDT 2013


  The option handling still seems over engineered. I would just store the current positional option state while parsing and use that to instantiate ELFInputElements directly. Groups should be an integral part of the InputGraph which contains InputElements as the resolver needs to iterate over these correctly.


================
Comment at: include/lld/Core/TargetInfo.h:240
@@ +239,3 @@
+  void appendLLVMOption(const char *opt) { _llvmOptions.push_back(opt); }
+  virtual void setInputGraph(std::unique_ptr<InputGraph> &inputGraph) {
+    _inputGraph = std::move(inputGraph);
----------------
This should not be a reference. Just pass by value.

================
Comment at: include/lld/Core/TargetInfo.h:243
@@ +242,3 @@
+  }
+  virtual const std::unique_ptr<InputGraph> &inputGraph() const {
+    return _inputGraph;
----------------
This should not return a unique_ptr. Just an InputGraph&.

================
Comment at: include/lld/Driver/Driver.h:41
@@ -38,3 +40,3 @@
 
-  /// Performs link using specified options.
+  /// Performs link using specified options
   static bool link(const TargetInfo &targetInfo,
----------------
Comments in LLVM contain full sentences and end with a period.

================
Comment at: include/lld/Driver/InputGraph.h:123
@@ +122,3 @@
+  /// Return the Element Type for an Input Element
+  virtual ElementType type() const { return _type; }
+
----------------
This is generally referred to as Kind in LLVM.

================
Comment at: include/lld/Driver/InputGraph.h:167
@@ +166,3 @@
+
+  static inline bool classof(const Option *) { return true; }
+};
----------------
Not needed.

================
Comment at: include/lld/Driver/InputGraph.h:192
@@ +191,3 @@
+
+  static inline bool classof(const ControlNode *) { return true; }
+
----------------
Not needed.

================
Comment at: include/lld/Driver/InputGraph.h:197
@@ +196,3 @@
+
+  /// \brief Iterators to iterate the
+  InputGraph::InputElementIterT begin() { return _elements.begin(); }
----------------
Incomplete sentence.

================
Comment at: include/lld/Driver/InputGraph.h:228
@@ +227,3 @@
+
+  static inline bool classof(const FileNode *) { return true; }
+
----------------
Not needed.

================
Comment at: include/lld/Driver/GnuLDInputGraph.h:67-69
@@ +66,5 @@
+    std::unique_ptr<ELFLinkerOptions> linkerOptions(new ELFLinkerOptions());
+    linkerOptions.get()->setForceLoad(_forceLoad);
+    linkerOptions.get()->setAsNeeded(_asNeeded);
+    linkerOptions.get()->copyLibraryPaths(_libraryPaths);
+    return std::move(linkerOptions);
----------------
Why .get()? unique_ptr has an overloaded operator ->.

================
Comment at: include/lld/Driver/GnuLDInputGraph.h:95
@@ +94,3 @@
+
+  static inline bool classof(ELFLinkerOptions *) { return true; }
+
----------------
Not needed.

================
Comment at: include/lld/Driver/GnuLDInputGraph.h:124
@@ +123,3 @@
+
+  static inline bool classof(const ELFFileNode *) { return true; }
+
----------------
Not needed.

================
Comment at: include/lld/Driver/GnuLDInputGraph.h:173
@@ +172,3 @@
+
+  static inline bool classof(const ELFGroup *) { return true; }
+
----------------
Not needed.

================
Comment at: include/lld/Driver/GnuLDInputGraph.h:152
@@ +151,3 @@
+
+  static inline bool classof(const ELFControlNode *) { return true; }
+
----------------
Not needed.

================
Comment at: lib/Driver/GnuLdDriver.cpp:350
@@ +349,3 @@
+      std::unique_ptr<InputElement> simpleControl =
+          std::move(gnuLinkerOptions.getInputElement(*inputArg, optionType));
+      if (controlNodeStack.empty())
----------------
This doesn't need a move. Return values are already rvalues.

================
Comment at: lib/Driver/GnuLdDriver.cpp:354
@@ +353,3 @@
+      else
+        ((llvm::dyn_cast<ControlNode>)(controlNodeStack.top()))
+            ->processInputElement(std::move(simpleControl));
----------------
This doesn't need the extra parens.


http://llvm-reviews.chandlerc.com/D1217



More information about the llvm-commits mailing list