Follow up of #277#pullrequestreview-4412854098. This adds ReadList helper and uses it to dedup the CustomReadField implementations for std::map, std::set, std::unordered_set, and std::vector, mirroring the existing BuildList helper on the build side.
I took the liberty of adding the commit f2a734a9c1515435743bba0fb9ef889687fc391c "type: reserve first when reading std::unordered_set" so it reserves the correct capacity first and then emplaces the values.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | xyzconstant |
| Concept ACK | ryanofsky |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
support branch for CI scripts, documentation, and examples by ryanofsky)If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
65 | - ReadDestEmplace( 66 | - TypeList<std::pair<const KeyLocalType, ValueLocalType>>(), [&](auto&&... args) -> auto& { 67 | - return *EmplacePiecewiseSafe(value, std::forward<decltype(args)>(args)...).first; 68 | - })); 69 | - } 70 | + ReadList(
In commit "proxy: add ReadList helper and dedup map/set/vector read handlers" (2bc6cb761fbccbea5600ec775f23274041aab599)
Not sure if possible but it might be good to pass read_dest instead of value to the ReadList function so callers don't need to call read_dest.update and repeat as much code. This would make ReadList and BuildList more similar in how they are called. Looks like this would require adding auto& value parameters to init and emplace functions, which could also be good because it will declare value closer to where it is used.
True, I will take a look on this approach
Forced push 245f7927742fad0ed3e5f2fe3b5fc246f58e2e78 to just rebase with master
Forced push 5d23f83e99de31cf932a58e51b29dd06240c70ac applying @ryanofsky suggestion. It reads cleaner and dedup more code.
Does this work here?
diff --git a/include/mp/type-vector.h b/include/mp/type-vector.h
index 25e74a6..cd269cb 100644
--- a/include/mp/type-vector.h
+++ b/include/mp/type-vector.h
@@ -50,14 +50,16 @@ decltype(auto) CustomReadField(TypeList<std::vector<bool>>,
Input&& input,
ReadDest&& read_dest)
{
- return read_dest.update([&](auto& value) {
- auto data = input.get();
- value.clear();
- value.reserve(data.size());
- for (auto item : data) {
- value.push_back(ReadField(TypeList<bool>(), invoke_context, Make<ValueField>(item), ReadDestTemp<bool>()));
- }
- });
+ return ReadList(
+ TypeList<bool>(), invoke_context, input, read_dest,
+ [&](auto& value, size_t size) {
+ value.clear();
+ value.reserve(size);
+ },
+ [&](auto& value, auto&& item) {
+ value.push_back(item);
+ return value.back();
+ });
}
} // namespace mp
oh I missed this case, thanks for pointing!
I tested your approach and it does compile and passes the tests but I think it is better to not add return value.back(); because bool is read via construct(), so the return value is always unused.
Also I think it is a good opportunity to add a comment to explain why there's this exception for std::vector<bool>.
What do you think?
diff --git a/include/mp/type-vector.h b/include/mp/type-vector.h
index 25e74a6..49ca3c2 100644
--- a/include/mp/type-vector.h
+++ b/include/mp/type-vector.h
@@ -43,6 +43,11 @@ decltype(auto) CustomReadField(TypeList<std::vector<LocalType>>,
});
}
+//! Overload CustomReadField for std::vector<bool>, which needs its own handler
+//! because its back() returns a proxy by value (std::vector<bool>::reference)
+//! rather than a reference, so it can't use the generic vector in-place emplace path
+//! that returns auto&. Not returning a reference is fine here: bool is read via
+//! construct(), not update(), so the return value is never used.
template <typename Input, typename ReadDest>
decltype(auto) CustomReadField(TypeList<std::vector<bool>>,
Priority<1>,
@@ -50,14 +55,15 @@ decltype(auto) CustomReadField(TypeList<std::vector<bool>>,
Input&& input,
ReadDest&& read_dest)
{
- return read_dest.update([&](auto& value) {
- auto data = input.get();
- value.clear();
- value.reserve(data.size());
- for (auto item : data) {
- value.push_back(ReadField(TypeList<bool>(), invoke_context, Make<ValueField>(item), ReadDestTemp<bool>()));
- }
- });
+ return ReadList(
+ TypeList<bool>(), invoke_context, input, read_dest,
+ [&](auto& value, size_t size) {
+ value.clear();
+ value.reserve(size);
+ },
+ [&](auto& value, auto&& item) {
+ value.push_back(item);
+ });
}
} // namespace mp
Great! This is definitely better, and the comment is on point.
Done on 1bfc59400b3d3a89012ac9591c38df121950185c.
Forced push 043b8a8fd375a77802d081e36c1ae87f324bf2cc applying @xyzconstant suggestion
tACK 043b8a8fd375a77802d081e36c1ae87f324bf2cc
Thanks for the updates @ViniciusCestarii.
This is a simple refactor that moves common code to ReadList, consistent with existing BuildList helper.
The changes are clean and can be merged as-is.