Change PathMappingList::FindFile to return an optional result (NFC)

[aprantl]

Summary

This is an NFC modernization refactoring that replaces the combination of a bool return + reference argument, with an Optional return value.

[aprantl]

Update to the correct patch

[aprantl]

Ping.

[shafik]

LGTM

lldb/source/Core/SourceManager.cpp

444

This whole comment reads awkward and does not totally make sense and ends with . then

lldb/source/Target/PathMappingList.cpp

231

Make sense to move FileSpec new_spec; declaration down to here.

[JDevlieghere]

LGTM modulo formatting

lldb/include/lldb/Target/PathMappingList.h

 //===-- PathMappingList.h ---------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_TARGET_PATHMAPPINGLIST_H
 #define LLDB_TARGET_PATHMAPPINGLIST_H
 
 #include <map>
 #include <vector>
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/Status.h"
 
 namespace lldb_private {
 
 class PathMappingList {
 public:
   typedef void (*ChangedCallback)(const PathMappingList &path_list,
                                   void *baton);
public:
   /// It makes sure the file exists by checking with the file system,
   /// so this call can be expensive if the remappings are on a network
   /// or are even on the local file system, so use this function
   /// sparingly (not in a tight debug info parsing loop).
   ///
   /// \param[in] orig_spec
   ///     The original source file path to try and remap.
   ///
-  /// \param[out] new_spec
-  ///     The newly remapped filespec that is guaranteed to exist.
-  ///
   /// \return
-  ///     /b true if \a orig_spec was successfully located and
-  ///     \a new_spec is filled in with an existing file spec,
-  ///     \b false otherwise.
-  bool FindFile(const FileSpec &orig_spec, FileSpec &new_spec) const;
+  ///     The newly remapped filespec that is guaranteed to exist.
+  llvm::Optional<FileSpec> FindFile(const FileSpec &orig_spec) const;
 
   uint32_t FindIndexForPath(ConstString path) const;
 
   uint32_t GetModificationID() const { return m_mod_id; }
 
 protected:
   typedef std::pair<ConstString, ConstString> pair;
   typedef std::vector<pair> collection;

lldb/source/Core/Module.cpp

 //===-- Module.cpp --------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Core/Module.h"
 
 #include "lldb/Core/AddressRange.h"
 #include "lldb/Core/AddressResolverFileLine.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/FileSpecList.h"
 #include "lldb/Core/Mangled.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/SearchFilter.h"
if (object_name != GetObjectName())
       return false;
   }
   return true;
 }
 
 bool Module::FindSourceFile(const FileSpec &orig_spec,
                             FileSpec &new_spec) const {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
-  return m_source_mappings.FindFile(orig_spec, new_spec);
+  if (auto remapped = m_source_mappings.FindFile(orig_spec)) {
+    new_spec = *remapped;
+    return true;
+  }
+  return false;
 }
 
 bool Module::RemapSourceFile(llvm::StringRef path,
                              std::string &new_path) const {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
   if (auto remapped = m_source_mappings.RemapPath(path)) {
     new_path = remapped->GetPath();
     return true;

lldb/source/Core/SourceManager.cpp

 //===-- SourceManager.cpp -------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Core/SourceManager.h"
 
 #include "lldb/Core/Address.h"
 #include "lldb/Core/AddressRange.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/FormatEntity.h"
 #include "lldb/Core/Highlighter.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleList.h"
if (target) {
             if (sc.comp_unit)
               m_file_spec = sc.comp_unit->GetPrimaryFile();
             m_mod_time = FileSystem::Instance().GetModificationTime(m_file_spec);
           }
         }
       }
       // Try remapping if m_file_spec does not correspond to an existing file.
       if (!FileSystem::Instance().Exists(m_file_spec)) {
-        FileSpec new_file_spec;
         // Check target specific source remappings first, then fall back to
         // modules objects can have individual path remappings that were
         // detected when the debug info for a module was found. then
-        if (target->GetSourcePathMap().FindFile(m_file_spec, new_file_spec) ||
-            target->GetImages().FindSourceFile(m_file_spec, new_file_spec)) {
-          m_file_spec = new_file_spec;
+        auto remapped = target->GetSourcePathMap().FindFile(m_file_spec);
+        if (!remapped) {
+          FileSpec new_spec;
+          if (target->GetImages().FindSourceFile(m_file_spec, new_spec))
+            remapped = new_spec;
+        }
+        if (remapped) {
+          m_file_spec = *remapped;
           m_mod_time = FileSystem::Instance().GetModificationTime(m_file_spec);
         }
       }
     }
   }
 
   if (m_mod_time != llvm::sys::TimePoint<>())
     m_data_sp = FileSystem::Instance().CreateDataBuffer(m_file_spec);

lldb/source/Symbol/LineEntry.cpp

 //===-- LineEntry.cpp -----------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Symbol/LineEntry.h"
 #include "lldb/Symbol/CompileUnit.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/Target.h"
 
 using namespace lldb_private;
 
 LineEntry::LineEntry()
     : range(), file(), is_start_of_statement(0), is_start_of_basic_block(0),
while (true) {
 
     break;
   }
   return complete_line_range;
 }
 
 void LineEntry::ApplyFileMappings(lldb::TargetSP target_sp) {
   if (target_sp) {
-    // Apply any file remappings to our file
-    FileSpec new_file_spec;
-    if (target_sp->GetSourcePathMap().FindFile(original_file, new_file_spec))
-      file = new_file_spec;
+    // Apply any file remappings to our file.
+    if (auto new_file_spec = target_sp->GetSourcePathMap().FindFile(original_file))
+      file = *new_file_spec;
   }
 }

lldb/source/Target/PathMappingList.cpp

 //===-- PathMappingList.cpp -----------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include <climits>
 #include <cstring>
 
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/PosixApi.h"
 #include "lldb/Target/PathMappingList.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/lldb-private-enumerations.h"
 
 using namespace lldb;
if (!path_ref.consume_front(it.second.GetStringRef()))
       continue;
     fixed.SetFile(it.first.GetStringRef(), FileSpec::Style::native);
     fixed.AppendPathComponent(path_ref);
     return true;
   }
   return false;
 }
 
-bool PathMappingList::FindFile(const FileSpec &orig_spec,
-                               FileSpec &new_spec) const {
+llvm::Optional<FileSpec>
+PathMappingList::FindFile(const FileSpec &orig_spec) const {
+  FileSpec new_spec;
   if (m_pairs.empty())
-    return false;
+    return {};
   
   std::string orig_path = orig_spec.GetPath();
     
   if (orig_path.empty())
-    return false;
+    return {};
       
   bool orig_is_relative = orig_spec.IsRelative();
 
   for (auto entry : m_pairs) {
     llvm::StringRef orig_ref(orig_path);
     llvm::StringRef prefix_ref = entry.first.GetStringRef();
     if (orig_ref.size() < prefix_ref.size())
       continue;
     // We consider a relative prefix or one of just "." to
     // mean "only apply to relative paths".
     bool prefix_is_relative = false;
     
     if (prefix_ref == ".") {
       prefix_is_relative = true;
       // Remove the "." since it will have been removed from the
       // FileSpec paths already.
       prefix_ref = prefix_ref.drop_front();
     } else {
       FileSpec prefix_spec(prefix_ref, FileSpec::Style::native);
       prefix_is_relative = prefix_spec.IsRelative();
     }
     if (prefix_is_relative != orig_is_relative)
       continue;
 
     if (orig_ref.consume_front(prefix_ref)) {
       new_spec.SetFile(entry.second.GetCString(), FileSpec::Style::native);
       new_spec.AppendPathComponent(orig_ref);
       if (FileSystem::Instance().Exists(new_spec))
-        return true;
+        return new_spec;
     }
   }
   
   new_spec.Clear();
-  return false;
+  return {};
 }
 
 bool PathMappingList::Replace(ConstString path,
                               ConstString new_path, bool notify) {
   uint32_t idx = FindIndexForPath(path);
   if (idx < m_pairs.size()) {
     ++m_mod_id;
     m_pairs[idx].second = new_path;