On Fri, 9 Aug 2024 16:05:00 GMT, Gerard Ziemski <gziem...@openjdk.org> wrote:
> > > Is there a concrete advantage here for using lambda expression when
> > > iterating committed regions? I'm asking because personally I find
> > > ```c++
> > > while ((committed_rgn = itr.next()) != nullptr) {
> > > print_committed_rgn(committed_rgn);
> > > }
> > > ```
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > simpler and more compact, hence easier to understand, than
> > > ```c++
> > > CommittedMemoryRegion cmr;
> > >
> > > VirtualMemoryTrackerWithTree::tree()->visit_committed_regions(reserved_rgn,
> > > &cmr, [&](CommittedMemoryRegion* crgn) {
> > > print_committed_rgn(crgn);
> > > return true;
> > > });
> > > ```
> >
> >
> > Hi Gerard, I'm the one who prefers passing in a lambda and introduced that
> > style, sorry :-).
> > First off, I think the lambda code should be simplified, it should be:
> > ```c++
> > VMWT::tree().visit_committed_regions_of(*reserved_rgn, [](const
> > CommittedMemoryRegion& crgn) {
> > print_committed_region(crgn));
> > return true;
> > });
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > I don't think that's too bad, right? The `return true` is a bit
> > unfortunate. It's for being able to stop early, which I agree that regular
> > iterators do better by just performing a `break;`.
> > I'll go ahead and say one nice thing too: If the body of the lambda is a
> > bit more complicated, then we can look at the capture list (the `[]` above)
> > to see what the lambda can alter in the outside scope. With a while-loop,
> > you really have to read the whole thing.
> > **The reason** that I write these kinds of iterators is that they're much
> > simpler to implement and maintain and, _to me_, STL-style iterators aren't
> > easier to read, it's about the same level of complexity.
> > I'll admit that your style of iterators (which are _not_ STL) is about the
> > same complexity, though I find it unfortunate that we have to write an
> > entire class for each iterator.
> > With the simplifications I made, is this style of iterator acceptable?
>
> hi Johan,
>
> Yes, this does look better. I looked at
> https://www.geeksforgeeks.org/when-to-use-lambda-expressions-instead-of-functions-in-cpp
> to see when one should consider using lambda and I like one particular
> scenario - improving readability by implementing the code locally, so one can
> see exactly what is going on without having to look inside a function. I
> think this is our use case scenario here.
>
> If only we didn't need all those `return` statements, I'd have clearly
> preferred lambda here in fact.
It's tedious to have to write `return true;` if you never `return false;` in
the function. I looked into fixing this with some metaprogramming, and I found
two C++14 solutions and one C++17 one. Here's the Godbolt:
https://godbolt.org/z/xGzoGYhf8
And here's the code, reproduced for posterity:
```c++
#include <iostream>
#include <vector>
// C++14 solution
// Here we take extra runtime cost for the void returning function
#define ENABLE_IF(...) \
std::enable_if_t<bool(__VA_ARGS__), int> = 0
template <typename F,
ENABLE_IF(std::is_same<bool, typename std::result_of<F(int
&)>::type>::value)>
void iterate(std::vector<int>& arr, F fun) {
for (int i = 0; i < arr.size(); i++) {
if (!fun(arr[i])) {
return;
}
}
}
template<typename F,
ENABLE_IF(std::is_void<typename std::result_of<F(int&)>::type>::value)>
void iterate(std::vector<int>& arr, F fun) {
iterate(arr, [&fun](int& i) {
fun(i);
return true;
});
}
// C++17 solution, notice that this compiles two functions: one for when
early_return is true and when for when it's not.
/*
template<typename F>
void iterate(std::vector<int>& arr, F fun) {
using ret_type = decltype(fun(arr[0])); // This doesn't actually execute
fun(arr[0]);
constexpr bool early_return = std::is_same<ret_type, bool>::value;
for (int i = 0; i < arr.size(); i++) {
if constexpr (early_return) {
if (!fun(arr[i])) {
return;
}
} else {
fun(arr[i]);
}
}
}
*/
// C++14 solution
// Here we have to make the two implementations ourselves.
/*
#define ENABLE_IF(...) \
std::enable_if_t<bool(__VA_ARGS__), int> = 0
template <typename F,
ENABLE_IF(std::is_same<bool, typename std::result_of<F(int
&)>::type>::value)>
void iterate(std::vector<int>& arr, F fun) {
for (int i = 0; i < arr.size(); i++) {
if (!fun(arr[i])) {
return;
}
}
}
template<typename F,
ENABLE_IF(std::is_void<typename std::result_of<F(int&)>::type>::value)>
void iterate(std::vector<int>& arr, F fun) {
for (int i = 0; i < arr.size(); i++) {
fun(arr[i]);
}
}
*/
int main() {
std::vector<int> my_array{0,1,2,3};
auto no_early = [](int& x) {
std::cout << x << " ";
};
auto early = [](int &x) {
if (x == 2) return false;
std::cout << x << " ";
return true;
};
iterate(my_array, no_early);
std::cout << std::endl;
iterate(my_array, early);
std::cout << std::endl;
}
> I honestly do not think we should be checking in 2 implementations, unless
> they are nicely hidden away. I do not like the way we manage them right now
> with 2 explicit `if` checks, each and every time we need to do something.
>
> If you could push the 2 implementations into a factory class, so that instead
> of:
>
> ```
> if (is_using_sorted_link_list())
> VirtualMemoryTracker::snapshot_thread_stacks();
> if (is_using_tree())
> VirtualMemoryTrackerWithTree::Instance::snapshot_thread_stacks();
> ```
>
> we have just:
>
> ` VirtualMemoryTracker::snapshot_thread_stacks();` and
>
> ```
> VirtualMemoryTracker::VirtualMemoryTracker(bool useLinkedList) {
> if (useLinkedList) {
> instance = new VirtualMemoryTrackerWithLinkedList()
> } else {
> instance = new VirtualMemoryTrackerWithLinkedList()
> }
> }
>
> VirtualMemoryTracker::snapshot_thread_stacks() {
> instance.snapshot_thread_stacks();
> }
> ```
>From reading the code, this might be a bit harder to do than preferred. We'd
>need some wrapping for the iterators, since they use two different styles.
>Wrapping the "old" iterators inside of the "new" style is possible of course.
The fork for JDK-24 is on December 5th. That means that we have at most 8 weeks
in the testing system to find and fix any bugs that we might have missed after
integration. To me, that feels a bit short. Either we wait after the fork to
integrate, or we integrate with two implementations and a `java` option for
choosing implementation. I'm open to other opinions.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2309938825
PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2378890343
