[Lldb-commits] [PATCH] D77327: [nfc] [lldb] 2/2: Introduce DWARF callbacks

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thanks for doing this. This looks more-or-less like what I was expecting. The need for the "bool" return value from the index functions is unfortunate (I did not anticipate that), but it does seem unavoidable.

Since none of these functions (I guess) are storing the callback function, could you replace std::function with llvm::function_ref ?



================
Comment at: lldb/include/lldb/Core/UniqueCStringMap.h:130-163
+  bool GetValues(ConstString unique_cstr,
+                 std::function<bool(T value)> callback) const {
+    for (const Entry &entry : llvm::make_range(std::equal_range(
+             m_map.begin(), m_map.end(), unique_cstr, Compare())))
+      if (callback(entry.value))
+        return true;
+
----------------
I'm not sure these functions are really needed. They have just one caller, and they'd be trivial if this class provided appropriate abstractions. Maybe just add begin/end/equal_range methods, so that one can write:
```
for (value: map / map.equal_range(str))
  stuff
```
?

(ideally, I'd like to have iterators instead of callbacks for the index classes too, but iterators and class hierarchies don't mix very well)


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h:79
   llvm::Optional<DIERef> ToDIERef(const DebugNames::Entry &entry);
-  void Append(const DebugNames::Entry &entry, DIEArray &offsets);
+  bool Append(const DebugNames::Entry &entry,
+              std::function<bool(DIERef ref)> callback);
----------------
Maybe rename this to `ProcessEntry` or something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77327