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

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added inline comments.


================
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.
----------------
JDevlieghere wrote:
> 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`).  
> IIUC we cannot pass

Your understanding is correct!

>  A DWARFDataExtractor is a DataExtractor so wouldn't that be fine?

Slicing isn't inherently bad, so long as the Derived constructor doesn't change the Base class in a way that leaves it in an invalid state after slicing. Does DWARFDataExtractor respect that? I honestly don't know, but even if it did, this is not future-proof.

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

I like this idea, let me give it a try in a separate patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153866