[Lldb-commits] [PATCH] D153866: [lldb] Use LLVM's implementation of AppleTables for apple_{names, namespaces}

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.
This revision is now accepted and ready to land.

LGTM, regardless of whether you decided the tackle the the DataExtractor issue. If you don't I would still encourage you to update/simplify/rephrase the comment.



================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp:26-29
+  // We could just pass "debug_str.getAsLLVM()", but that would cause slicing:
+  // DWARFDataExtractor->DataExtractor
+  // since AppleAcceleratorTable ctors take non-DWARF DataExtractors for the
+  // string section.
----------------
I was a little confused by this comment. IIUC we cannot pass `debug_str.getAsLLVM()` to the `AppleAcceleratorTable` because it takes the DataExtractor by value and that would cause slicing. But why is that a problem though? A `DWARFDataExtractor` is a `DataExtractor` so wouldn't that be fine? 

Alternatively, should we have another `getAsLLVM()` helper that returns a `DataExtractor` (and rename the current one to something like `getAsLLVMDWARF`).  


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp:235
     llvm::function_ref<bool(DWARFDIE die)> callback) {
-  if (!m_apple_names_up)
-    return;
-
-  DWARFMappedHash::DIEInfoArray hash_data;
-  m_apple_names_up->AppendAllDIEsThatMatchingRegex(regex, hash_data);
-  DWARFMappedHash::ExtractDIEArray(hash_data,
-                                   DIERefCallback(callback, regex.GetText()));
+  return GetGlobalVariables(regex, callback);
 }
----------------
fdeazeve wrote:
> In case you are thinking: "This method doesn't make any sense!", you are right.
> 
> This is called "GetFunctions" but doesn't return only functions, callers are expected to check if the DIE is a function or not. I thought about changing this, but didn't want to add any behavior changes in the patch.
👍


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153866