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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
