[Lldb-commits] [PATCH] D56814: [Reproducers] Refactor reproducer info

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

This looks much better. LGTM, just make sure to do something with the lower_bound search, as I don't think that's right.



================
Comment at: include/lldb/Utility/Reproducer.h:58-59
 
+  virtual const char *GetName() const = 0;
+  virtual const char *GetFile() const = 0;
+
----------------
return StringRef ?


================
Comment at: include/lldb/Utility/Reproducer.h:164
   FileSpec m_root;
+  std::vector<std::string> m_files;
   bool m_loaded;
----------------
This might even be a vector of StringRefs, if you're ok with saying that the Provider class is responsible for these strings with sufficient lifetime. In the current setup it kind of already is because there's no telling who will use the returned c strings and when.


================
Comment at: source/Utility/Reproducer.cpp:218-219
   assert(m_loaded);
-
-  auto it = m_provider_info.find(name);
-  if (it == m_provider_info.end())
-    return llvm::None;
-
-  return it->second;
+  auto it = std::lower_bound(m_files.begin(), m_files.end(), file.str());
+  return it != m_files.end();
 }
----------------
This doesn't seem right. This will only return false if `file` is lexicographically after all files in the `m_files` array. Perhaps you meant `return it != m_files.end() && *it == file;` ?


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56814/new/

https://reviews.llvm.org/D56814