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

Shankar Kalpathi Easwaran shankarke at gmail.com
Mon Oct 7 21:07:11 PDT 2013



================
Comment at: include/lld/Driver/InputGraph.h:330
@@ +329,3 @@
+/// \brief Represents Internal Input files
+class SimpleFileNode : public InputElement {
+public:
----------------
Rui Ueyama wrote:
> 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.
Its not intended for Archives. Its intended for any internal file that you want to add. For example ImplicitFiles/InternalFiles.

================
Comment at: include/lld/Driver/InputGraph.h:59
@@ +58,3 @@
+    END
+  };
+
----------------
Rui Ueyama wrote:
> 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.
Isnt the current model simplistic ?

================
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
----------------
Rui Ueyama wrote:
> 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.
This has been there from the initial implementation of InputGraph. I want to keep this as-is.

================
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);
----------------
Rui Ueyama wrote:
> 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.
makes sense.


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



More information about the llvm-commits mailing list