[Lldb-commits] [PATCH] D59562: [SymbolFileDWARF] Introduce DWARFContext

[Zachary Turner via Phabricator via lldb-commits]

zturner marked 2 inline comments as done.
zturner added inline comments.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:50
+
+const DWARFDataExtractor *DWARFContext::getOrLoadArangesData() {
+  return LoadOrGetSection(m_module, m_dwarf_data, eSectionTypeDWARFDebugAranges,
----------------
clayborg wrote:
> rename to "getArangesData()"? Would it be better to return an Optional<DWARFDataExtractor>?
I actually prefer to keep the name as `getOrLoad`.  `get` functions often sound like they don't modify anything, but this way it's clear that lazy evaluation will occur, which might be important to the caller for some reason or another.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:24
+  // forward.
+  extractor.emplace();
+
----------------
clayborg wrote:
> clayborg wrote:
> > Don't we want to put an empty DWARFDataExtractor in here? Maybe this code should be:
> > 
> > ```
> > const SectionList *section_list = module.GetSectionList();
> >   if (!section_list)
> >     return nullptr;
> > 
> >   auto section_sp = section_list->FindSectionByType(section_type, true);
> >   if (!section_sp)
> >     return nullptr;
> > 
> >   extractor.emplace();
> >   if (section_sp->GetSectionData(*extractor) == 0)
> >     extractor = llvm::None;
> >   return extractor.getPointer();
> > ```
> Or use a local DWARFDataExtractor and move it into "extractor" if we succeed?
> 
> ```
> DWARFDataExtractor data;
> if (section_sp->GetSectionData(data) > 0)
>     extractor = std::move(data);
> ```
The suggested code here has a bug.  If you say 

```
extractor = llvm::None;
return extractor.getPointer();
```

it will assert, because there is no pointer, it's uninitialized.  When i write `extractor.emplace();` it initializes it with a default constructed `DataExtractor`, the idea being that if we try this function and it fails for any reason, we should not try the function again, because it already failed once, and so we should just "cache" the fact that it failed and immediately return.  By having a default constructed `DataExtractor` and changing the fast path exit condition to also return null in the case where the size is 0, we ensure that we only ever do work once, regardless of whether that work fails or succeeds.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:40
+
+const DWARFDataExtractor *DWARFContext::getOrLoadArangesData() {
+  return LoadOrGetSection(m_module, eSectionTypeDWARFDebugAranges,
----------------
clayborg wrote:
> Why do we care about "getOrLoadArangesData()"? What not just "getArangesData()"?
I like to emphasize in the name that the function might do work.  People are trained to think of `get` functions as read-only, but it's not the case here and so the idea is to make sure that nobody can misinterpret the behavior.


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

https://reviews.llvm.org/D59562