[llvm-commits] [PATCH] lld driver

[Michael Spencer]

================
Comment at: include/lld/Core/ErrorOr.h:10-12
@@ +9,5 @@
+///
+/// \file
+///
+/// Provides ErrorOr<T> smart pointer.
+///
----------------
Sean Silva wrote:
> Since this class seems generally useful, it is probably worth mentioning here why this file is not in libSupport.
It's c++11. Although it's still useful for reference and value types without c++11.

================
Comment at: include/lld/Core/ErrorOr.h:109-110
@@ +108,4 @@
+/// of an operation. It is pointer-like in that operator * and operator ->
+/// return T& and T* respectively. Special handling is provided for reference
+/// types. The value is only valid if the error is errc::success. Otherwise
+/// accessing the value has undefined behavior (will assert if asserts are
----------------
Sean Silva wrote:
> Ok... *what* special handling? The sentence as it stands might as well be "you're going to have to read the source to understand how this class behaves when `T` is a reference type".
I'll add that. Just have to figure out how to word it well.

================
Comment at: include/lld/Driver/LinkerInvocation.h:26-27
@@ +25,4 @@
+
+  /// \brief Perform the link.
+  void operator()();
+
----------------
Sean Silva wrote:
> {execute,do}Invocation() would be self-documenting. It seems pointless to use the "special" operator() name here.
Any named member would be redundant. The reason this is a class and not a function is that I expect the need to extend it later.

================
Comment at: include/lld/Driver/LinkerOptions.h:35-36
@@ +34,4 @@
+  Native,
+  Object,
+  llvm,
+  Script
----------------
Sean Silva wrote:
> Why the weird case for `llvm` compared with all the others?
Probably from typing all the includes and namespace qualifiers. I'll fix it.

================
Comment at: include/lld/Driver/LinkerOptions.h:119
@@ +118,3 @@
+  std::string _entrySymbol;
+  unsigned _relocatable : 1;
+
----------------
Sean Silva wrote:
> `bool`? or a comment explaining this weird choice.
I expect to add more bitfields. unsigned is needed because MSVC doesn't handle bitfields with other types nicely.

================
Comment at: lib/Driver/CoreOptions.td:3
@@ +2,3 @@
+
+def target : Separate<["-"], "target">, HelpText<"Target tripple to link for">;
+
----------------
João Matos wrote:
> Small typo on "triple" here.
fixed.

================
Comment at: lib/Driver/Drivers.cpp:34
@@ +33,3 @@
+#include "CoreOptions.inc"
+  LastOption
+#undef OPTION
----------------
João Matos wrote:
> Maybe this should be OPT_LAST, to be consistent with OPT_INVALID?
It can actually be removed. It's there because that's what clang did.

================
Comment at: lib/Driver/Drivers.cpp:53-54
@@ +52,4 @@
+public:
+  CoreOptTable() : OptTable(InfoTable, sizeof(InfoTable) / sizeof(InfoTable[0])){}
+};
+}
----------------
Sean Silva wrote:
> llvm::array_lengthof(); here and below
done.

================
Comment at: lib/Driver/Drivers.cpp:135-137
@@ +134,5 @@
+    // Copy input args.
+    for (llvm::opt::arg_iterator it = _inputArgs->filtered_begin(ld::OPT_INPUT),
+                                 ie = _inputArgs->filtered_end();
+                                 it != ie; ++it) {
+      newArgs->AddPositionalArg(*it, _core.getOption(core::OPT_INPUT),
----------------
Sean Silva wrote:
> For a separate patch: could there just be a `filtered()` or `filtered_range()` or `filtered_all()` that returns a range? It kills me to see the `for (... it = ..., ie = ...` when you have a range-for available.
Yes, having ranges would be lovely. Someone just has to implement the range library for llvm.

================
Comment at: lib/Driver/Drivers.cpp:155
@@ +154,3 @@
+  case Flavor::ld:
+    return std::unique_ptr<Driver>(new LDDriver(defaultTargetTriple));
+  case Flavor::core:
----------------
Sean Silva wrote:
> You may want to put in a `make_unique()` for this and other places.
> 
> Herb Sutter calls the absence of this function a bug in the standard.
> 
> Sample implementation
> 
> ```template<typename T, typename ...Args>
> std::unique_ptr<T> make_unique( Args&& ...args )
> {
>     return std::unique_ptr<T>( new T( std::forward<Args>(args)... ) );
> }```
Yeah, I thought about that, but didn't feel it was worth it yet. I suppose I can add it to llvm/Support.

================
Comment at: lib/Driver/LDOptions.td:4
@@ +3,3 @@
+def flavor : Separate<["-"], "flavor">;
+def target : Separate<["-"], "target">, HelpText<"Target tripple to link for">;
+
----------------
João Matos wrote:
> Small typo on "triple" here.
fixed.

================
Comment at: lib/Driver/LinkerInvocation.cpp:40-48
@@ +39,11 @@
+
+      if (error_code ec = input.getBuffer()) {
+        llvm::errs() << "Failed to read file: " << input.getPath() << ": "
+                     << ec.message() << "\n";
+        return;
+      }
+
+      std::vector<std::unique_ptr<File>> files;
+      if (llvm::error_code ec = reader->readFile(
+            input.getBuffer()->getBufferIdentifier(), files)) {
+        llvm::errs() << "Failed to read file: " << input.getPath() << ": "
----------------
Sean Silva wrote:
> This `input.getBuffer()` really freaked me out. First I was like "wtf, why is `input.getBuffer()` returning an `error_code` above but something dereferenceable below", then I realized it was using `ErrorOr<>` with an implicit conversion, then I was like "why is it ok to skip the error check and directly dereference it in the second call?" then I realized the first call is doing the check for the second one.
> 
> Bottom line: put the result of `input.getBuffer()` in a local variable, the same way you do below for `target->getWriter()`.
> 
> Btw that idiom with `ErrorOr<>` (*with* the local variable) is really elegant. Much much nicer than the "return an `error_code` and real return value gets put in an out-param" style in the main LLVM codebase.
Fixed.

================
Comment at: lib/Driver/Targets.cpp:11-13
@@ +10,5 @@
+/// \file
+///
+/// Concrete instances of the Target interface.
+///
+//===----------------------------------------------------------------------===//
----------------
João Matos wrote:
> Sean Silva wrote:
> > Maybe call the file "ConcreteTargets.cpp". It's kind of annoying to see "Target.cpp" and "Targets.cpp", where nothing besides the "plural" relationship is apparent. "ConcreteTargets.cpp" makes the relationship obvious.
> Since Clang (Basic/Targets.cpp) and LLVM (llvm/Config/Targets.def) already use Targets for this, I'd prefer to keep the same terminology.
Yep, I picked the same names that LLVM and Clang use, even if they are a bit weird.


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