[ADT] Add DenseSet(llvm::from_t, Range) #131832
Conversation
This patch adds a constructor of the form: DenseSet Set(llvm::from_range, Range) while forward-porting std::from_range from c++23.
|
@llvm/pr-subscribers-llvm-adt Author: Kazu Hirata (kazutakahirata) ChangesThis patch adds a constructor of the form: DenseSet Set(llvm::from_range, Range) while forward-porting std::from_range from c++23. Full diff: https://github.com/llvm/llvm-project/pull/131832.diff 3 Files Affected:
diff --git a/llvm/include/llvm/ADT/DenseSet.h b/llvm/include/llvm/ADT/DenseSet.h
index ac766e778df0d..5642f1400bc39 100644
--- a/llvm/include/llvm/ADT/DenseSet.h
+++ b/llvm/include/llvm/ADT/DenseSet.h
@@ -17,6 +17,7 @@
#include "llvm/ADT/ADL.h"
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/DenseMapInfo.h"
+#include "llvm/ADT/STLForwardCompat.h"
#include "llvm/Support/MathExtras.h"
#include "llvm/Support/type_traits.h"
#include <cstddef>
@@ -78,6 +79,10 @@ class DenseSetImpl {
insert(Elems.begin(), Elems.end());
}
+ template <typename Range>
+ DenseSetImpl(llvm::from_range_t, Range &&R)
+ : DenseSetImpl(adl_begin(R), adl_end(R)) {}
+
bool empty() const { return TheMap.empty(); }
size_type size() const { return TheMap.size(); }
size_t getMemorySize() const { return TheMap.getMemorySize(); }
diff --git a/llvm/include/llvm/ADT/STLForwardCompat.h b/llvm/include/llvm/ADT/STLForwardCompat.h
index 6afe3610b257f..10c274ea8e23e 100644
--- a/llvm/include/llvm/ADT/STLForwardCompat.h
+++ b/llvm/include/llvm/ADT/STLForwardCompat.h
@@ -67,6 +67,12 @@ template <typename Enum>
return static_cast<std::underlying_type_t<Enum>>(E);
}
+// A tag for constructors accepting ranges.
+struct from_range_t {
+ explicit from_range_t() = default;
+};
+inline constexpr from_range_t from_range{};
+
} // namespace llvm
#endif // LLVM_ADT_STLFORWARDCOMPAT_H
diff --git a/llvm/unittests/ADT/DenseSetTest.cpp b/llvm/unittests/ADT/DenseSetTest.cpp
index 8fc2bab44b124..3f668b831a450 100644
--- a/llvm/unittests/ADT/DenseSetTest.cpp
+++ b/llvm/unittests/ADT/DenseSetTest.cpp
@@ -33,6 +33,12 @@ TEST(DenseSetTest, DoubleEntrySetTest) {
EXPECT_EQ(0u, set.count(2));
}
+TEST(DenseSetTest, CtorRange) {
+ constexpr unsigned Args[] = {3, 1, 2};
+ llvm::DenseSet<unsigned> set(llvm::from_range, Args);
+ EXPECT_THAT(set, ::testing::UnorderedElementsAre(1, 2, 3));
+}
+
TEST(DenseSetTest, InsertRange) {
llvm::DenseSet<unsigned> set;
constexpr unsigned Args[] = {3, 1, 2};
|
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
while forward-porting std::from_range from c++23.
Could you describe (or link to) what std::from_range is and explain why we'd want to use it in llvm?
Sure: https://en.cppreference.com/w/cpp/ranges/from_range
|
|
I don't think we should use or support C++ ranges in LLVM. |
IMO this should be a part of the PR description.
+1, I'm more interested in learning what concrete problems this solves and if we can benefit from it without buying into the whole ecosystem of ranges from C++23. |
|
Might be worth showing, in the test case, where the disambiguation is useful? I guess the relevant issues arise when using CTAD? Or something else? |
|
@kuhar I recently added into: But then I thought it would be nice to declare the variable and initialize it at the same time: Now, the C++23 way of doing this appears to be: That's how I've arrived at @dwblaikie IIUC, the need for If Now, our By the way, as a data point, our whereas the equivalent code with Getting back to this PR, I'm totally fine with dropping |
|
Thanks for the context. Do we have any alternatives for llvm containers? For example, could we follow what we do with |
|
@kuhar I think the way history played out there is that we added to_vector for this use case, but then still ended up adding the ArrayRef overload to SmallVector later because the ergonomics of having the ctor are just better. I think that if there are no technical issues with it, having a ctor accepting a range is a good idea. |
|
OK, so the use of from_t is for consistency, if nothing else - I'm OK with that? Having range based ctors would be nice, and having them work the same way as standard containers will work seems fine/good to me. Other people feel strongly otherwise? |
I'm fine with that too, but I'm not entirely convinced that this is a better convention than a helper function or a 'static constructor function' like |
Even if we don't adopt ranges wholesale, it seems like these ctors (on standard types) would be pretty harmless/likely to get used in LLVM once it's available? (I haven't looked at ranges in detail so I'm not sure if there's a clear cut-line & whether this falls on one side or the other) Like I could imagine a rule like "don't use anything from the header or std::ranges namespace" which this ctor wouldn't fall afoul of. |
|
Hi @kazutakahirata, I thought about this over the week and I think this proposal is the most reasonable one after all. The issue with adding a range constructor is that it may lead to unintentional copies like with With CTAD, |
Wouldn't using an explicit ctor avoid that? That's what SmallVector does. |
I may be missing a point, but do we need to worry about marking the proposed constructor? Requiring |
I think we guard against incompatible element types with the arrayref version but not against different small sizes with the llvm-project/llvm/include/llvm/ADT/SmallVector.h Lines 1230 to 1254 in adcaf17 from_range is simpler/safer in comparison.
|
Exactly. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/16925 Here is the relevant piece of the build log for the reference |
6 participants
This patch adds a constructor of the form:
DenseSet Set(llvm::from_range, Range)
while forward-porting std::from_range from c++23.