[lld-macho][nfc] Remove unnecessary use of Optional<T*>

[oontvoo]

Summary

In all of these cases, the functions could simply return a nullptr instead of {}.
There is no case where Optional<nullptr> has a special meaning.

[thakis]

I guess the idea was that the Optional forces you to "null" check, while the pointer doesn't? Fine to land if you want, but fine to not land too imho.

[int3]

I like the safety that Optional<T*> gives, but I'm also aware that we aren't consistent about using it in our codebase (and I like consistency), so I'm fine with this

[oontvoo]

In D103489#2792029, @thakis wrote:

I guess the idea was that the Optional forces you to "null" check, while the pointer doesn't? Fine to land if you want, but fine to not land too imho.

In D103489#2792127, @int3 wrote:

I like the safety that Optional<T*> gives, but I'm also aware that we aren't consistent about using it in our codebase (and I like consistency), so I'm fine with this

Thanks! IMO, null is already a sentinel value, so why not use it? :-)

lld/MachO/Driver.h

 
 void parseLCLinkerOption(InputFile *, unsigned argc, StringRef data);
 
 std::string createResponseFile(const llvm::opt::InputArgList &args);
 
 // Check for both libfoo.dylib and libfoo.tbd (in that order).
 llvm::Optional<std::string> resolveDylibPath(llvm::StringRef path);
 
-llvm::Optional<DylibFile *> loadDylib(llvm::MemoryBufferRef mbref,
-                                      DylibFile *umbrella = nullptr,
-                                      bool isBundleLoader = false);
+DylibFile *loadDylib(llvm::MemoryBufferRef mbref, DylibFile *umbrella = nullptr,
 
 // Search for all possible combinations of `{root}/{name}.{extension}`.
 // If \p extensions are not specified, then just search for `{root}/{name}`.
 llvm::Optional<llvm::StringRef>
 findPathCombination(const llvm::Twine &name,
                     const std::vector<llvm::StringRef> &roots,
                     ArrayRef<llvm::StringRef> extensions = {""});
 

lld/MachO/Driver.cpp

case file_magic::archive: {
     break;
   }
   case file_magic::macho_object:
     newFile = make<ObjFile>(mbref, getModTime(path), "");
     break;
   case file_magic::macho_dynamically_linked_shared_lib:
   case file_magic::macho_dynamically_linked_shared_lib_stub:
   case file_magic::tapi_file:
-    if (Optional<DylibFile *> dylibFile = loadDylib(mbref))
-      newFile = *dylibFile;
+    if (DylibFile *dylibFile = loadDylib(mbref))
+      newFile = dylibFile;
     break;
   case file_magic::bitcode:
     newFile = make<BitcodeFile>(mbref);
     break;
   case file_magic::macho_executable:
   case file_magic::macho_bundle:
     // We only allow executable and bundle type here if it is used
     // as a bundle loader.
     if (!isBundleLoader)
       error(path + ": unhandled file type");
-    if (Optional<DylibFile *> dylibFile =
-            loadDylib(mbref, nullptr, isBundleLoader))
-      newFile = *dylibFile;
+    if (DylibFile *dylibFile = loadDylib(mbref, nullptr, isBundleLoader))
+      newFile = dylibFile;
     break;
   default:
     error(path + ": unhandled file type");
   }
   if (newFile) {
     // printArchiveMemberLoad() prints both .a and .o names, so no need to
     // print the .a name here.
     if (config->printEachFile && magic != file_magic::archive)

lld/MachO/DriverUtils.cpp

else
     depTracker->logFileNotFound(location);
   return {};
 }
 
 // It's not uncommon to have multiple attempts to load a single dylib,
 // especially if it's a commonly re-exported core library.
 static DenseMap<CachedHashStringRef, DylibFile *> loadedDylibs;
 
-Optional<DylibFile *> macho::loadDylib(MemoryBufferRef mbref,
-                                       DylibFile *umbrella,
-                                       bool isBundleLoader) {
+DylibFile *macho::loadDylib(MemoryBufferRef mbref, DylibFile *umbrella,
   CachedHashStringRef path(mbref.getBufferIdentifier());
   DylibFile *file = loadedDylibs[path];
   if (file)
     return file;
 
   file_magic magic = identify_magic(mbref.getBuffer());
   if (magic == file_magic::tapi_file) {
     Expected<std::unique_ptr<InterfaceFile>> result = TextAPIReader::get(mbref);
     if (!result) {
       error("could not load TAPI file at " + mbref.getBufferIdentifier() +
             ": " + toString(result.takeError()));
-      return {};
+      return nullptr;
     }
     file = make<DylibFile>(**result, umbrella, isBundleLoader);
   } else {
     assert(magic == file_magic::macho_dynamically_linked_shared_lib ||
            magic == file_magic::macho_dynamically_linked_shared_lib_stub ||
            magic == file_magic::macho_executable ||
            magic == file_magic::macho_bundle);
     file = make<DylibFile>(mbref, umbrella, isBundleLoader);

lld/MachO/InputFiles.cpp

void ObjFile::parseDebugInfo() {
   const DWARFContext::compile_unit_range &units = ctx->compile_units();
   // FIXME: There can be more than one compile unit per object file. See
   // PR48637.
   auto it = units.begin();
   compileUnit = it->get();
 }
 
 // The path can point to either a dylib or a .tbd file.
-static Optional<DylibFile *> loadDylib(StringRef path, DylibFile *umbrella) {
+static DylibFile *loadDylib(StringRef path, DylibFile *umbrella) {
   Optional<MemoryBufferRef> mbref = readFile(path);
   if (!mbref) {
     error("could not read dylib file at " + path);
-    return {};
+    return nullptr;
   }
   return loadDylib(*mbref, umbrella);
 }
 
 // TBD files are parsed into a series of TAPI documents (InterfaceFiles), with
 // the first document storing child pointers to the rest of them. When we are
 // processing a given TBD file, we store that top-level document in
 // currentTopLevelTapi. When processing re-exports, we search its children for
 // potentially matching documents in the same TBD file. Note that the children
 // themselves don't point to further documents, i.e. this is a two-level tree.
 //
 // Re-exports can either refer to on-disk files, or to documents within .tbd
 // files.
-static Optional<DylibFile *>
-findDylib(StringRef path, DylibFile *umbrella,
+DylibFile *findDylib(StringRef path, DylibFile *umbrella,
                      const InterfaceFile *currentTopLevelTapi) {
   if (path::is_absolute(path, path::Style::posix))
     for (StringRef root : config->systemLibraryRoots)
       if (Optional<std::string> dylibPath =
               resolveDylibPath((root + path).str()))
         return loadDylib(*dylibPath, umbrella);
 
   // TODO: Expand @loader_path, @executable_path, @rpath etc, handle -dylib_path
 
   if (currentTopLevelTapi) {
     for (InterfaceFile &child :
          make_pointee_range(currentTopLevelTapi->documents())) {
       assert(child.documents().empty());
       if (path == child.getInstallName())
         return make<DylibFile>(child, umbrella);
     }
   }
 
   if (Optional<std::string> dylibPath = resolveDylibPath(path))
     return loadDylib(*dylibPath, umbrella);
 
-  return {};
+  return nullptr;
 }
 
 // If a re-exported dylib is public (lives in /usr/lib or
 // /System/Library/Frameworks), then it is considered implicitly linked: we
 // should bind to its symbols directly instead of via the re-exporting umbrella
 // library.
 static bool isImplicitlyLinked(StringRef path) {
   if (!config->implicitDylibs)
     return false;
 
   if (path::parent_path(path) == "/usr/lib")
     return true;
 
   // Match /System/Library/Frameworks/$FOO.framework/**/$FOO
   if (path.consume_front("/System/Library/Frameworks/")) {
     StringRef frameworkName = path.take_until([](char c) { return c == '.'; });
     return path::filename(path) == frameworkName;
   }
 
   return false;
 }
 
 void loadReexport(StringRef path, DylibFile *umbrella,
                   const InterfaceFile *currentTopLevelTapi) {
-  Optional<DylibFile *> reexport =
-      findDylib(path, umbrella, currentTopLevelTapi);
+  DylibFile *reexport = findDylib(path, umbrella, currentTopLevelTapi);
   if (!reexport)
     error("unable to locate re-export with install name " + path);
   else if (isImplicitlyLinked(path))
-    inputFiles.insert(*reexport);
+    inputFiles.insert(reexport);
 }
 
 DylibFile::DylibFile(MemoryBufferRef mb, DylibFile *umbrella,
                      bool isBundleLoader)
     : InputFile(DylibKind, mb), refState(RefState::Unreferenced),
       isBundleLoader(isBundleLoader) {
   assert(!isBundleLoader || !umbrella);
   if (umbrella == nullptr)
for (uint32_t i = 0, n = hdr->ncmds; i < n; ++i) {
     // FIXME: What about LC_LOAD_UPWARD_DYLIB, LC_LAZY_LOAD_DYLIB,
     // LC_LOAD_WEAK_DYLIB, LC_REEXPORT_DYLIB (..are reexports from dylibs with
     // MH_NO_REEXPORTED_DYLIBS loaded for -flat_namespace)?
     if (config->namespaceKind == NamespaceKind::flat &&
         cmd->cmd == LC_LOAD_DYLIB) {
       const auto *c = reinterpret_cast<const dylib_command *>(cmd);
       StringRef dylibPath =
           reinterpret_cast<const char *>(c) + read32le(&c->dylib.name);
-      Optional<DylibFile *> dylib = findDylib(dylibPath, umbrella, nullptr);
+      DylibFile *dylib = findDylib(dylibPath, umbrella, nullptr);
       if (!dylib)
         error(Twine("unable to locate library '") + dylibPath +
               "' loaded from '" + toString(this) + "' for -flat_namespace");
     }
   }
 }
 
 DylibFile::DylibFile(const InterfaceFile &interface, DylibFile *umbrella,