[llvm-commits] [PATCH] lld driver

Sean Silva silvas at purdue.edu
Wed Dec 5 01:18:20 PST 2012


  This code is really nice. Mostly small stuff.

  Documentation is weak. You should plan on a `docs/DriverArchitecture.rst` or something like that in a future patch.


================
Comment at: include/lld/Driver/Driver.h:50-51
@@ +49,4 @@
+
+  virtual std::unique_ptr<llvm::opt::DerivedArgList>
+    transform(llvm::ArrayRef<const char *const> args) = 0;
+
----------------
random: You have really good taste in source layout.

================
Comment at: include/lld/Driver/LinkerInvocation.h:26-27
@@ +25,4 @@
+
+  /// \brief Perform the link.
+  void operator()();
+
----------------
{execute,do}Invocation() would be self-documenting. It seems pointless to use the "special" operator() name here.

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

================
Comment at: lib/Driver/Drivers.cpp:155
@@ +154,3 @@
+  case Flavor::ld:
+    return std::unique_ptr<Driver>(new LDDriver(defaultTargetTriple));
+  case Flavor::core:
----------------
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)... ) );
}```

================
Comment at: include/lld/Driver/LinkerOptions.h:119
@@ +118,3 @@
+  std::string _entrySymbol;
+  unsigned _relocatable : 1;
+
----------------
`bool`? or a comment explaining this weird choice.

================
Comment at: include/lld/Core/ErrorOr.h:10-12
@@ +9,5 @@
+///
+/// \file
+///
+/// Provides ErrorOr<T> smart pointer.
+///
----------------
Since this class seems generally useful, it is probably worth mentioning here why this file is not in libSupport.

================
Comment at: include/lld/Driver/LinkerOptions.h:35-36
@@ +34,4 @@
+  Native,
+  Object,
+  llvm,
+  Script
----------------
Why the weird case for `llvm` compared with all the others?

================
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() << ": "
----------------
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.

================
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
----------------
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".

================
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),
----------------
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.

================
Comment at: lib/Driver/Targets.cpp:11-13
@@ +10,5 @@
+/// \file
+///
+/// Concrete instances of the Target interface.
+///
+//===----------------------------------------------------------------------===//
----------------
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.

================
Comment at: tools/lld/lld.cpp:96-99
@@ +95,6 @@
+  // On mac treat ld as ld64.
+#ifdef __APPLE__
+  if (flavor == Driver::Flavor::ld)
+    flavor = Driver::Flavor::ld64;
+#endif
+
----------------
As per discussion on IRC, I think that this check is really weird since in this one instance the `Flavor` enum is being used to represent "the name of the program as it was invoked on the command line", whereas everywhere else it represents "what linker should we behave as" (so this check is saying "if we should behave as gnu ld, we should behave as ld64", which is nonsense since they are mutually exclusive).

I would really like to see this comment changed to say "on mac, if we are invoked as ld, behave as ld64". Bonus points for saying "we briefly violate the semantics of the Flavor enum and use it here to represent just the name of the program that we were invoked as". idk why this is bothering me so much, but it really is.

================
Comment at: tools/lld/lld.cpp:121-122
@@ +120,4 @@
+
+  Driver::Flavor iHazAFlavor = selectFlavor(argc, argv);
+  if (iHazAFlavor == Driver::Flavor::invalid)
+    return 1;
----------------
no comment...


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



More information about the llvm-commits mailing list