This is an archive of the discontinued LLVM Phabricator instance.

RangeMap.h: merge RangeDataArray and RangeDataVector
ClosedPublic

Authored by labath on Dec 31 2018, 3:09 AM.

Details

Summary

The main difference between the classes was supposed to be the fact that
one is backed by llvm::SmallVector, and the other by std::vector.
However, over the years, they have accumulated various other differences
too.

This essentially removes the std::vector version, as that is pretty much
identical to llvm::SmallVector<T, 0>, and combines their interfaces. It
does not attempt to do a more significant refactoring, even though there
is still a lot of duplication in this file, as it is hard to tell which
quirk of some API is depended on by somebody (and, a previous, more
ambitious attempt at this in D16769 has failed).

I also add some tests, including one which demonstrates one of the
quirks/bugs of the API I have noticed in the process.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Dec 31 2018, 3:09 AM
davide added a subscriber: davide.Jan 2 2019, 12:23 AM
davide added inline comments.
unittests/Core/RangeMapTest.cpp
52–54 ↗(On Diff #179754)

I think this is a bug in the implementation.

This revision is now accepted and ready to land.Jan 2 2019, 12:25 AM
labath marked an inline comment as done.Jan 2 2019, 2:00 AM
labath added inline comments.
unittests/Core/RangeMapTest.cpp
52–54 ↗(On Diff #179754)

I agree. I think this can be fixed if we change the while loop in FindEntryThatContains to loop until the current range start address goes below the lookup address. If that's the behavior we want from this function, that is.

tberghammer accepted this revision.Jan 2 2019, 2:46 AM

LGTM

include/lldb/Core/RangeMap.h
636 ↗(On Diff #179754)

Would it make sense to have a default value of 0 for N so people don't have to specify it explicitly?

source/Plugins/Process/elf-core/ProcessElfCore.h
140 ↗(On Diff #179754)

Should this be 1 to keep the previous semantics?

labath marked 2 inline comments as done.Jan 2 2019, 4:28 AM
labath added inline comments.
include/lldb/Core/RangeMap.h
636 ↗(On Diff #179754)

I don't have an clear opinion on that. On one hand, 0 seems like a perfectly reasonable default, but on the other SmallVector doesn't have a default either. Everyone typedefs these anyway, so it doesn't really matter.

source/Plugins/Process/elf-core/ProcessElfCore.h
140 ↗(On Diff #179754)

If we wanted to be strict then yes, but it seemed weird to have different sizes for the two VMRange maps, when both of them will (hopefully) hold the same number of entries. My guess is that the author here meant "the smallest number possible" and didn't realize that 0 is valid too (or maybe it wasn't valid at that time).

clayborg added inline comments.Jan 2 2019, 9:59 AM
include/lldb/Core/RangeMap.h
636 ↗(On Diff #179754)

Can we default N to be zero?

template <typename B, typename S, typename T, unsigned N=0>

Then remove the zero parameter from many of the changes in this patch?

include/lldb/Symbol/DWARFCallFrameInfo.h
117 ↗(On Diff #179754)

We can remove the last zero parameter if we default its value in the template definition

include/lldb/Symbol/LineTable.h
233 ↗(On Diff #179754)

We can remove the last zero parameter if we default its value in the template definition

include/lldb/Symbol/Symtab.h
150 ↗(On Diff #179754)

We can remove the last zero parameter if we default its value in the template definition

source/Plugins/Process/elf-core/ProcessElfCore.h
139–140 ↗(On Diff #179754)

We can remove the last zero parameter if we default its value in the template definition

142 ↗(On Diff #179754)

We can remove the last zero parameter if we default its value in the template definition

source/Plugins/Process/mach-core/ProcessMachCore.h
133–134 ↗(On Diff #179754)

We can remove the last zero parameter if we default its value in the template definition

136 ↗(On Diff #179754)

Ditto

source/Plugins/SymbolFile/DWARF/DWARFDebugAranges.h
22 ↗(On Diff #179754)

Should this default to zero instead of 1? Not many DWARFDebugAranges will have just a single address.

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
443 ↗(On Diff #179754)

We can remove the last zero parameter if we default its value in the template definition

source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
159 ↗(On Diff #179754)

We can remove the last zero parameter if we default its value in the template definition

302 ↗(On Diff #179754)

We can remove the last zero parameter if we default its value in the template definition

labath updated this revision to Diff 180027.Jan 3 2019, 3:28 AM

Set N=0 default parameter, and remove the size specification from all usages
(including the one in DWARFDebugArranges, where N was 1).

clayborg accepted this revision.Jan 3 2019, 10:58 AM
This revision was automatically updated to reflect the committed changes.