Add contains() to some //base classes [chromium/src : main]
Victor Vianna (Gerrit)
Victor Vianna voted and added 3 comments![]( "Open in Gerrit")
Votes added by Victor Vianna
| Auto-Submit | +1 |
3 comments
#include "base/containers/contains.h"This one is still used, so the CL keeps it
Related details
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Victor Vianna (Gerrit)
Victor Vianna voted and added 2 comments![]( "Open in Gerrit")
Votes added by Victor Vianna
| Auto-Submit | +1 |
2 comments
#include "base/containers/contains.h"This one is still used, so the CL keeps it
Related details
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 3 comments![]( "Open in Gerrit")
constexpr bool contains(const key_type& key) const {This should support heterogeneous lookup.
small_map<std::unordered_map<int, int>> m;Please pick a container other than std::unordered_map here. I don't care strongly which one, but we have guidance that discourages new std::unordered_map use.
bool contains(const Key& key) const { return find(key) != end(); }Ditto: this was probably an oversight when we introduced variant map, but we should not persist this mistake for the new contains() method.
Related details
- Victor Vianna
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Victor Vianna (Gerrit)
Victor Vianna voted and added 3 comments![]( "Open in Gerrit")
Votes added by Victor Vianna
| Auto-Submit | +1 |
3 comments
This should support heterogeneous lookup.
I just learned about heterogenous lookup, hopefully I got it right.
Please pick a container other than std::unordered_map here. I don't care strongly which one, but we have guidance that discourages new std::unordered_map use.
Done
bool contains(const Key& key) const { return find(key) != end(); }Ditto: this was probably an oversight when we introduced variant map, but we should not persist this mistake for the new contains() method.
VariantMap currently has no EqualKey template param
https://source.chromium.org/chromium/chromium/src/+/main:base/containers/variant_map.h;l=74;drc=1b049924c0f9432e31d6987e085a656c66bf94dc
Are you proposing I copy this to VariantMap?
https://source.chromium.org/chromium/chromium/src/+/main:base/containers/small_map.h;l=105-151;drc=134ef9afcaff9644ce859d61752d3ef51b924eb6
Related details
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 2 comments![]( "Open in Gerrit")
constexpr iterator find(const T& key) {You shouldn't change this; while we didn't hit it here, there are times when the non-template overload would be used (typically when template deduction would otherwise fail)
I'd just leave this as-is and add a TODO amd address it in a followup.
bool contains(const Key& key) const { return find(key) != end(); }Victor ViannaDitto: this was probably an oversight when we introduced variant map, but we should not persist this mistake for the new contains() method.
VariantMap currently has no EqualKey template param
https://source.chromium.org/chromium/chromium/src/+/main:base/containers/variant_map.h;l=74;drc=1b049924c0f9432e31d6987e085a656c66bf94dcAre you proposing I copy this to VariantMap?
https://source.chromium.org/chromium/chromium/src/+/main:base/containers/small_map.h;l=105-151;drc=134ef9afcaff9644ce859d61752d3ef51b924eb6
Ah... maybe it's a bit hard for `VariantMap` because one subtype is ordered and the other is not.
Let's just ignore this, since it's supposed to be removed in M145.
Related details
- Victor Vianna
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Victor Vianna (Gerrit)
Victor Vianna added 2 comments![]( "Open in Gerrit")
constexpr bool contains(const key_type& key) const {
return find(key) != end();
}Do I even need this given the one below?
constexpr iterator find(const T& key) {You shouldn't change this; while we didn't hit it here, there are times when the non-template overload would be used (typically when template deduction would otherwise fail)
I'd just leave this as-is and add a TODO amd address it in a followup.
Sorry, the suggestion is not clear to me. If I don't have templated find(), how am I gonna implement contains()? Do you want me to duplicate+adapt the find's method body in contains?
If you just want a `constexpr iterator find(const key_type& key)`, I can do that in this patch already
Related details
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
constexpr iterator find(const T& key) {Victor ViannaYou shouldn't change this; while we didn't hit it here, there are times when the non-template overload would be used (typically when template deduction would otherwise fail)
I'd just leave this as-is and add a TODO amd address it in a followup.
Sorry, the suggestion is not clear to me. If I don't have templated find(), how am I gonna implement contains()? Do you want me to duplicate+adapt the find's method body in contains?
If you just want a `constexpr iterator find(const key_type& key)`, I can do that in this patch already
Oh right, `find()` is missing this and that's what `contains()` uses. It's probably easiest to duplicate it then; I don't think it's specified if the wrapped map is ordered or not.
Otherwise I would have suggested the trick that flat_tree uses: https://chromium-review.googlesource.com/c/chromium/src/+/7163836
Related details
- Victor Vianna
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Victor Vianna (Gerrit)
Victor Vianna voted and added 3 comments![]( "Open in Gerrit")
Votes added by Victor Vianna
| Auto-Submit | +1 |
3 comments
constexpr bool contains(const key_type& key) const {
return find(key) != end();
}Do I even need this given the one below?
From the other thread, it seems like, yes, we want a non-template version too
Victor ViannaYou shouldn't change this; while we didn't hit it here, there are times when the non-template overload would be used (typically when template deduction would otherwise fail)
I'd just leave this as-is and add a TODO amd address it in a followup.
Daniel ChengSorry, the suggestion is not clear to me. If I don't have templated find(), how am I gonna implement contains()? Do you want me to duplicate+adapt the find's method body in contains?
If you just want a `constexpr iterator find(const key_type& key)`, I can do that in this patch already
Oh right, `find()` is missing this and that's what `contains()` uses. It's probably easiest to duplicate it then; I don't think it's specified if the wrapped map is ordered or not.
Otherwise I would have suggested the trick that flat_tree uses: https://chromium-review.googlesource.com/c/chromium/src/+/7163836
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng voted![]( "Open in Gerrit")
- Victor Vianna
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Victor Vianna (Gerrit)
Victor Vianna voted Commit-Queue+2![]( "Open in Gerrit")
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Add contains() to some //base classes
Add to small_map, variant_map and cstring_view. Then use the new methods
to replace some base::Contains() calls.
- M base/containers/small_map.h
- M base/containers/small_map_unittest.cc
- M base/containers/variant_map.h
- M base/containers/variant_map_unittest.cc
- M base/files/file_util.cc
- M base/strings/cstring_view.h
- M base/strings/cstring_view_unittest.cc
- M components/page_load_metrics/renderer/page_timing_metrics_sender.cc
- M components/performance_manager/resource_attribution/cpu_measurement_monitor.cc
- M components/performance_manager/resource_attribution/cpu_measurement_monitor_unittest.cc
- M content/zygote/zygote_linux.cc
- M extensions/common/manifest_handler_registry.cc
- M gpu/command_buffer/service/framebuffer_manager.cc
- M media/gpu/chromeos/mailbox_video_frame_converter.cc
- M media/gpu/v4l2/v4l2_video_decoder_backend_stateless.cc
- M media/gpu/vaapi/vaapi_video_encode_accelerator.cc
- M mojo/public/cpp/bindings/binder_map.h
- M mojo/public/cpp/bindings/lib/multiplex_router.cc
- M mojo/public/cpp/bindings/receiver_set.h
- M pdf/pdfium/pdfium_font_win.cc
Code-Review: +1 by Daniel Cheng
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |