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

[Rui Ueyama]

  I did not have time to review this patch so doing it now. Looks like it's generally going to a right direction. Some part of code needs cleanup and simplification though. Because it's already submitted, I'm going to address my comments myself, but here's my comments anyway.


================
Comment at: include/lld/Core/Error.h:76
@@ +75,3 @@
+
+struct inputGraph_error {
+  enum _ {
----------------
Does inputGraph_error follow the LLVM naming convention? It's a mix of camel case and underscore.

================
Comment at: include/lld/Core/LinkingContext.h:46
@@ -46,3 +45,3 @@
 /// to the ELF Reader and Writer.
-class LinkingContext : public Reader {
+class LinkingContext {
 public:
----------------
Thank you for doing this. I didn't like LinkingContext inherits Reader.

================
Comment at: include/lld/Core/LinkingContext.h:238
@@ -238,3 +237,3 @@
   /// the actual input files.
-  virtual std::vector<std::unique_ptr<lld::File> > createInternalFiles();
+  virtual bool createInternalFiles(std::vector<std::unique_ptr<File> > &) const;
 
----------------
What is the meaning of the return value? If the reason to move the vector from return value to parameter is to avoid copying overhead, you could use std::move.

================
Comment at: include/lld/Core/LinkingContext.h:307
@@ +306,3 @@
+  /// no more files to be processed an appropriate inputGraph_error is returned.
+  virtual ErrorOr<File &> nextFile() const;
+
----------------
It shouldn't be const-qualified. Subclasses may want to update their internal state in nextFile(). PECOFFReaderContext would update its internal flags as it interprets its ".drectve" section, for example.

================
Comment at: include/lld/Core/LinkingContext.h:314
@@ +313,3 @@
+  /// atoms.
+  virtual void setResolverState(int32_t resolverState) const;
+
----------------
I'd use typedef'd type instead of int32_t. At least, using an unsigned type would be better.

================
Comment at: include/lld/Driver/CoreInputGraph.h:45
@@ +44,3 @@
+  llvm::error_code parse(const LinkingContext &ctx) {
+    auto filePath = path(ctx);
+    if (!filePath &&
----------------
Please don't use auto for this kind of variable as other programmers can't get the type by looking at it.

================
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);
----------------
Why do we handle only no_such_file_or_directory? Can't other type of error happen?

================
Comment at: include/lld/Driver/CoreInputGraph.h:54
@@ +53,3 @@
+
+    if ((ec = llvm::MemoryBuffer::getFileOrSTDIN(*filePath, opmb)))
+      return ec;
----------------
I'd write it as "if (error_code ec = ...)".

================
Comment at: include/lld/Driver/CoreInputGraph.h:60
@@ +59,3 @@
+
+    ec = _ctx.getDefaultReader().parseFile(_buffer, _files);
+    return ec;
----------------
You could just return the return value.

================
Comment at: include/lld/Driver/DarwinInputGraph.h:45
@@ +44,3 @@
+  llvm::error_code parse(const LinkingContext &ctx) {
+    auto filePath = path(ctx);
+    if (!filePath &&
----------------
Ditto

================
Comment at: include/lld/Driver/GnuLdInputGraph.h:75
@@ +74,3 @@
+  llvm::error_code parse(const LinkingContext &ctx) {
+    auto filePath = path(ctx);
+    if (!filePath &&
----------------
Ditto

================
Comment at: include/lld/Core/LinkingContext.h:315
@@ +314,3 @@
+  /// atoms.
+  virtual void setResolverState(int32_t resolverState) const;
+
----------------
A bitmask variable should be unsigned.

================
Comment at: include/lld/Driver/GnuLdInputGraph.h:88
@@ +87,3 @@
+    std::unique_ptr<MemoryBuffer> mb(opmb.take());
+    _buffer = std::move(mb);
+
----------------
The above code is repeated in all the flavors of input graphs. These need to be consolidated.

================
Comment at: include/lld/Driver/GnuLdInputGraph.h:75
@@ +74,3 @@
+  llvm::error_code parse(const LinkingContext &ctx, raw_ostream &diagnostics) {
+    auto filePath = path(ctx);
+    if (!filePath &&
----------------
Use of auto in this context make this code hard to read. It's better to write the explicit type.

================
Comment at: include/lld/Driver/GnuLdInputGraph.h:156
@@ +155,3 @@
+    return *_files[_nextFileIndex++];
+  }
+
----------------
This method seems to be a copy-n-paste of DarwinInputGraph's. This code duplication does not look good.

================
Comment at: include/lld/Driver/InputGraph.h:90
@@ +89,3 @@
+  // \brief Does the inputGraph contain any elements
+  int64_t size() const { return _inputArgs.size(); }
+
----------------
Should return a value of type size_t?

================
Comment at: include/lld/Driver/InputGraph.h:59
@@ +58,3 @@
+    END
+  };
+
----------------
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.

================
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
----------------
What is this filed? If it's dead we should remove.

================
Comment at: include/lld/Driver/InputGraph.h:330
@@ +329,3 @@
+/// \brief Represents Internal Input files
+class SimpleFileNode : public InputElement {
+public:
----------------
Does SimpleFileNode contains multiple files? The name is confusing.

================
Comment at: include/lld/Driver/WinLinkInputGraph.h:55
@@ +54,3 @@
+    std::unique_ptr<MemoryBuffer> mb(opmb.take());
+    _buffer = std::move(mb);
+
----------------
Looks like it's also duplicate.


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