8303621: BitMap::iterate should support lambdas and other function objects #12876
Conversation
|
👋 Welcome back kbarrett! A progress list of the required criteria for merging this PR into |
|
@kimbarrett The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
|
@kimbarrett This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
| // treated as if it always returned true, i.e. the iteration never | ||
| // terminates early. Otherwise, the result must be convertible to bool. | ||
| // | ||
| // - cl->do_bit(i) is a valid expression whose result is convertible to bool. |
There was a problem hiding this comment.
This description was an intermediate that I wasn't finished with, and isn't correct since it doesn't
describe cl->do_bit(i) as potentially returning void, which is supported by the implementation.
I seem to have lost the final version, and will need to rewrite it.
| template <typename Function> | ||
| inline bool BitMap::iterate(Function function, idx_t beg, idx_t end) const { | ||
| for (idx_t index = beg; true; ++index) { | ||
| index = get_next_one_offset(index, end); | ||
| if (index >= end) { | ||
| return true; | ||
| } else if (!cl->do_bit(index)) { | ||
| } else if (!IterateInvoker<decltype(function(index))>()(function, index)) { | ||
| return false; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
IMHO, this is an eyesore that I prefer if it could be hidden away a bit. Could we create an invoke function that that calls this, and change the code to:
template <typename Function>
inline bool BitMap::iterate(Function function, idx_t beg, idx_t end) const {
for (idx_t index = beg; true; ++index) {
index = get_next_one_offset(index, end);
if (index >= end) {
return true;
} else if (!invoke(function, index)) {
return false;
}
}
}
And also add a comment why we need IterateInvoker?
There was a problem hiding this comment.
I added a local function object so that template clutter wasn't all right in the middle of the function.
Making it a member function instead seemed to just push around the deck chairs.
Added comment, and also fixed a lurking, probably never to be hit, bug that we were stopping early if
ReturnType::operator! returned true, rather than if the converted to bool result was false.
There was a problem hiding this comment.
Here's an alternative, using member function templates instead of the
partially specialized IterateInvoker function object. This doesn't seem
obviously better to me. Other ideas I looked at involved messy SFINAE, some
with trailing return types (a not yet approved for HotSpot C++11 feature).
template<typename Function, typename Dispatch>
static bool iterate_invoke_aux(Function function, idx_t index, Dispatch*) {
return function(index);
}
template<typename Function>
static bool iterate_invoke_aux(Function function, idx_t index, void*) {
function(index);
return true;
}
template<typename Function>
static bool iterate_invoke(Function function, idx_t index) {
using ReturnType = decltype(function(index));
return iterate_invoke_aux(function, index, static_cast<ReturnType*>(nullptr));
}
|
/integrate |
|
Going to push as commit 008c5eb. |
openjdk
bot
removed
ready
|
@kimbarrett Pushed as commit 008c5eb. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Please review this enhancement of BitMap::iterate to support lambdas and other
function objects as the operation being applied to the set bit indices. Some
for-loops using BitMap::get_next_one_offset have been changed to use
BitMap::iterate with a lambda.
(One reason for changing the for-loops is that I'm considering a change to the
get_next_one_offset API, and reducing the number of direct uses will simplify
that.)
For convenience, the function can either return void (always iterate over the
whole range) or bool (stop iteration if returns false). Iteration using
closure objects invoked via a do_bit member function are now implemented by
being wrapped in a lambda, so get the same convenience. (Though, of course,
if the closure is derived from BitMapClosure then do_bit returns bool.)
The unit tests are written as "early" tests, not requiring an initialized VM.
They also avoid any C heap allocation (even though C heap allocation has very
early support). This is done to minimize the requirements for running the
tests, since BitMap is used in a lot of places. This attempts to run these
tests before uses. (Yes, I know about JDK-8257226; maybe that will be fixed
someday.) (Some existing BitMap gtests should be modified to do similarly; see
JDK-8303636.)
Testing:
mach5 tier1-7
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/12876/head:pull/12876$ git checkout pull/12876Update a local copy of the PR:
$ git checkout pull/12876$ git pull https://git.openjdk.org/jdk pull/12876/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 12876View PR using the GUI difftool:
$ git pr show -t 12876Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12876.diff