Change PathMappingList::RemapPath 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.

[shafik]

lldb/source/Core/Module.cpp

1604

Why not also have this return an Optional?

[aprantl]

lldb/source/Core/Module.cpp

1604

That would be perfectly reasonable. I'll add a review for that patch.

[aprantl]

Patch for Module is in https://reviews.llvm.org/D104724.

[aprantl]

Ping.

[jingham]

LGTM.

If the return type of the new function is not too awful to type, maybe don't use auto? It says it's returning a path, which is usually a string, not a FileSpec... But that's more a matter of taste.

lldb/source/Core/Module.cpp

1607–1611

RemapPath doesn't make it obvious you are getting a FileSpec back. Is the actual return something awful to type?

[aprantl]

if the return type of the new function is not too awful to type, maybe don't use auto? It says it's returning a path, which is usually a string, not a FileSpec... But that's more a matter of taste.

I think that's generally advice. In this particular case, the code gets deleted by a subsequent patch in this stack anyway.

lldb/include/lldb/Target/PathMappingList.h

public:
   ///
   /// Remaps \a path if any source remappings match. This function
   /// does NOT stat the file system so it can be used in tight loops
   /// where debug info is being parsed.
   ///
   /// \param[in] path
   ///     The original source file path to try and remap.
   ///
-  /// \param[out] new_path
-  ///     The newly remapped filespec that is may or may not exist.
-  ///
   /// \return
-  ///     /b true if \a path was successfully located and \a new_path
-  ///     is filled in with a new source path, \b false otherwise.
-  bool RemapPath(llvm::StringRef path, std::string &new_path) const;
+  ///     The remapped filespec that may or may not exist on disk.
+  llvm::Optional<FileSpec> RemapPath(llvm::StringRef path) const;
   bool RemapPath(const char *, std::string &) const = delete;
 
   bool ReverseRemapPath(const FileSpec &file, FileSpec &fixed) const;
 
   /// Finds a source file given a file spec using the path remappings.
   ///
   /// Tries to resolve \a orig_spec by checking the path remappings.
   /// It makes sure the file exists by checking with the file system,

lldb/source/Core/Module.cpp

 }
 
 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);
 }
 
 bool Module::RemapSourceFile(llvm::StringRef path,
                              std::string &new_path) const {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
-  return m_source_mappings.RemapPath(path, new_path);
+  if (auto remapped = m_source_mappings.RemapPath(path)) {
+    new_path = remapped->GetPath();
+    return true;
+  }
+  return false;
 }
 
 void Module::RegisterXcodeSDK(llvm::StringRef sdk_name, llvm::StringRef sysroot) {
   XcodeSDK sdk(sdk_name.str());
   ConstString sdk_path(HostInfo::GetXcodeSDKPath(sdk));
   if (!sdk_path)
     return;
   // If the SDK changed for a previously registered source path, update it.

lldb/source/Target/PathMappingList.cpp

if (!m_pairs.empty())
     ++m_mod_id;
   m_pairs.clear();
   if (notify && m_callback)
     m_callback(*this, m_callback_baton);
 }
 
 bool PathMappingList::RemapPath(ConstString path,
                                 ConstString &new_path) const {
-  std::string remapped;
-  if (RemapPath(path.GetStringRef(), remapped)) {
-    new_path.SetString(remapped);
+  if (llvm::Optional<FileSpec> remapped = RemapPath(path.GetStringRef())) {
+    new_path.SetString(remapped->GetPath());
     return true;
   }
   return false;
 }
 
-bool PathMappingList::RemapPath(llvm::StringRef path,
-                                std::string &new_path) const {
+llvm::Optional<FileSpec>
+PathMappingList::RemapPath(llvm::StringRef path) const {
   if (m_pairs.empty() || path.empty())
-    return false;
+    return {};
   LazyBool path_is_relative = eLazyBoolCalculate;
   for (const auto &it : m_pairs) {
     auto prefix = it.first.GetStringRef();
     if (!path.consume_front(prefix)) {
       // Relative paths won't have a leading "./" in them unless "." is the
       // only thing in the relative path so we need to work around "."
       // carefully.
       if (prefix != ".")
         continue;
       // We need to figure out if the "path" argument is relative. If it is,
       // then we should remap, else skip this entry.
       if (path_is_relative == eLazyBoolCalculate) {
         path_is_relative =
             FileSpec(path).IsRelative() ? eLazyBoolYes : eLazyBoolNo;
       }
       if (!path_is_relative)
         continue;
     }
     FileSpec remapped(it.second.GetStringRef());
     remapped.AppendPathComponent(path);
-    new_path = remapped.GetPath();
-    return true;
+    return remapped;
   }
-  return false;
+  return {};
 }
 
 bool PathMappingList::ReverseRemapPath(const FileSpec &file, FileSpec &fixed) const {
   std::string path = file.GetPath();
   llvm::StringRef path_ref(path);
   for (const auto &it : m_pairs) {
     if (!path_ref.consume_front(it.second.GetStringRef()))
       continue;