Page MenuHomePhabricator

[adt] SparseBitVector::test() should be const

Authored by dsanders on Oct 19 2018, 11:59 AM.



Re-worked SparseBitVector's most-recently-used-word caching (CurrElementIter)
such that SparseBitVector::test() can be made const. This came up when
attempting to test individual bits in a SparseBitVector which was a member of a
const object.

The cached iterator has no bearing on the externally visible state, it's merely
a performance optimization. Therefore it has been made mutable and
FindLowerBound() has been split into a const and non-const function
(FindLowerBound/FindLowerBoundConst) for the const/non-const

Diff Detail


Event Timeline

dsanders created this revision.Oct 19 2018, 11:59 AM
rtereshin accepted this revision.Oct 31 2018, 10:34 AM
rtereshin added a subscriber: rtereshin.

LGTM, thanks for doing this!

310 ↗(On Diff #170230)

Technically, const qualifier is a part of the member function signature, so no need for a different name. If called from another const function, like test, the const version of this function will be called regardless of the object itself being const or not.

But honestly, I like making the difference explicit in this specific case, it draws attention, and it's probably a good thing as FindLowerBoundImpl needs to be used or modified with care so we don't accidentally leak a non-const iterator from a const object in the future.

In the end, I'm fine with both, a different name and the same.

This revision is now accepted and ready to land.Oct 31 2018, 10:34 AM


310 ↗(On Diff #170230)

Yep, that's the reason I used separate names in this particular case. The const_casts are safe so long as we only call the const version in a const context and the non-const version in a non-const context. It's difficult to tell which you're calling when the names are the same.

dsanders edited the summary of this revision. (Show Details)Oct 31 2018, 12:53 PM
This revision was automatically updated to reflect the committed changes.