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

443–444

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

lldb/source/Target/PathMappingList.cpp

232

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

[JDevlieghere]

LGTM modulo formatting

lldb/include/lldb/Target/PathMappingList.h

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

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

if (target) {
             sc_list.GetContextAtIndex(0, sc);
             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;
+        // Check target specific source remappings (i.e., the
+        // target.source-map setting), then fall back to the module
+        // specific remapping (i.e., the .dSYM remapping dictionary).
+        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

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

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 {
   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)) {
+      FileSpec new_spec;
       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;