[PATCH] [lld][inputGraph] Implement flavor specific resolver

Rui Ueyama ruiu at google.com
Mon Oct 7 20:39:48 PDT 2013


  I have already made some changes to address the comments above, so let me finish them first.


================
Comment at: include/lld/Driver/InputGraph.h:188
@@ +187,3 @@
+  int64_t _ordinal;        // The ordinal value
+  int64_t _weight;         // Weight of the file
+  int32_t _resolveState;   // The resolve state
----------------
Shankar Kalpathi Easwaran wrote:
> Rui Ueyama wrote:
> > What is this filed? If it's dead we should remove.
> setWeight function handles this and will be used shortly. I can discuss with you on IRC tomorrow.
I mean it's better to add a field at the same time as the feature that uses the field is implemented. Sometimes it makes sense to split a patch, but this simple field is not the case.

================
Comment at: include/lld/Driver/InputGraph.h:59
@@ +58,3 @@
+    END
+  };
+
----------------
Shankar Kalpathi Easwaran wrote:
> Rui Ueyama wrote:
> > I feel like InputGraph is overly generalized. It's generality reflected the need that Resolver have to handle input files in a platform-neutral manner, and input files can be complicated because of --{start,end}-group, -z something, etc. Now these complexities are hidden behind each flavor's getNextFile() method. That would give us a chance to reduce its complexity by representing input files using simpler data structure.
> > 
> > This is not a review comment that needs to be resolved soon though.
> It has to be a common infrastructure, that is used over all flavors. Its important to have these functions to insert input elements at any place in the graph. 
My point is the way to abstract linker input as InputGraph and InputElement itself can be simplified.

================
Comment at: include/lld/Driver/CoreInputGraph.h:47
@@ +46,3 @@
+    if (!filePath &&
+        error_code(filePath) == llvm::errc::no_such_file_or_directory)
+      return make_error_code(llvm::errc::no_such_file_or_directory);
----------------
Shankar Kalpathi Easwaran wrote:
> Rui Ueyama wrote:
> > Why do we handle only no_such_file_or_directory? Can't other type of error happen?
> path can return only no_such_file_or_directory. I cant think of other errors.
We should have an assert after this method to assure that.

================
Comment at: include/lld/Driver/InputGraph.h:330
@@ +329,3 @@
+/// \brief Represents Internal Input files
+class SimpleFileNode : public InputElement {
+public:
----------------
Shankar Kalpathi Easwaran wrote:
> Rui Ueyama wrote:
> > Does SimpleFileNode contains multiple files? The name is confusing.
> It resembles SimpleFile. It does contain multiple files, but is usually a place holder for files that are internally added by the Linker. See Driver. 
> 
I'd name it SimpleArchiveFileNode.


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



More information about the llvm-commits mailing list