Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2944523002:
Improving flat containers interface. (Closed)
DescriptionImproving flat containers interface.
This patch adds a few features to base::flat_set/base::flat_map.
1) C++14 overloads of search operations.
Now count, find, equal_range, lower_bound, upper_bound accept any type comparable
with key_type if the comparison is transparent. Comparison is considered to be
transparent if it has `is_transparent` member typedef
(http://en.cppreference.com/w/cpp/utility/functional/less_void)
We removed non-templated versions, because they were mainly for backward
compatibility in extremely rare cases.
2) Templated version of erase.
This version was in the original proposal but later was removed as a result of a
bug report, because it changed overload resolution for `erase(iterator)`. We
provide an explicit `erase(iterator)` overload that addresses this issue.
BUG=682254
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2944523002
Cr-Commit-Position: refs/heads/master@{#488865}
Committed: https://chromium.googlesource.com/chromium/src/+/99a394d8dfee489d2a45308681f2bbbbf28ee6f8
Patch Set 1 #Patch Set 2 : Removing redundant tests. #
Total comments: 3
Patch Set 3 : Review, round 1. #
Total comments: 9
Patch Set 4 : Review, round 2. #
Total comments: 23
Patch Set 5 : Review, round 3. #
Total comments: 10
Patch Set 6 : Review, round 4. #
Total comments: 24
Patch Set 7 : Review, round 5. #
Total comments: 7
Patch Set 8 : Review, round 6. #Patch Set 9 : Hopefuly, fix for git issue. #
Total comments: 9
Patch Set 10 : Review, round 7. #Patch Set 11 : Compilation: stricter type checking. #Patch Set 12 : Compilation 2. #Patch Set 13 : Rebase. #Patch Set 14 : Compilation 2. #Patch Set 15 : Compilation 3. #
Total comments: 4
Patch Set 16 : Minimizing target_auto_attacher diff #
Total comments: 4
Patch Set 17 : Review, minimizing diff 2. #
Total comments: 2
Patch Set 18 : Review docs. #Patch Set 19 : Rebase. #Patch Set 20 : Rebase 2. #Patch Set 21 : Other platforms compilation. #Patch Set 22 : Other platforms compilation 2. #Messages
Total messages: 162 (81 generated)
Description was changed from ========== Improving flat containers interface. This patch adds a few features to base::flat_set/base::flat_map. 1) C++14 overloads of search operations. Now count, find, equal_range, lower_bound, upper_bound accept any type comarable with key_type, if comparison is transparent. Comparison is considered to be transparent if it has is_transparent member typedef (http://en.cppreference.com/w/cpp/utility/functional/less_void) 2) In addition we suport templated version of erase. This version was in the original proposal but later was removed as a result of a bug report because it changed overload resolution for erase(iterator). We provide an explicit erase(iterator) overload that addresses this issue. 3) Our variation of less operator supports comparing between a smart pointer and a pointer which allows to use flat_set<unique_ptr<T>> directly instead of doing a common workarond flat_map<T*, unique_ptr<T>>. In order to provide these features, we implement: std::enable_it_t from C++14. std::conditional_t from C++14. detection idiom (partial) from C++17. BUG=682254 ========== to ========== Improving flat containers interface. This patch adds a few features to base::flat_set/base::flat_map. 1) C++14 overloads of search operations. Now count, find, equal_range, lower_bound, upper_bound accept any type comparable with key_type if the comparison is transparent. Comparison is considered to be transparent if it has `is_transparent` member typedef (http://en.cppreference.com/w/cpp/utility/functional/less_void) 2) Templated version of erase. This version was in the original proposal but later was removed as a result of a bug report because it changed overload resolution for `erase(iterator)`. We provide an explicit `erase(iterator)` overload that addresses this issue. 3) less supports comparing between a smart pointer and a pointer. This allows to use flat_set<unique_ptr<T>> directly instead of doing a common workarond flat_map<T*, unique_ptr<T>>. In order to provide these features, we implement: std::enable_it_t from C++14. std::conditional_t from C++14. detection idiom (partial) from C++17. BUG=682254 ==========
Description was changed from ========== Improving flat containers interface. This patch adds a few features to base::flat_set/base::flat_map. 1) C++14 overloads of search operations. Now count, find, equal_range, lower_bound, upper_bound accept any type comparable with key_type if the comparison is transparent. Comparison is considered to be transparent if it has `is_transparent` member typedef (http://en.cppreference.com/w/cpp/utility/functional/less_void) 2) Templated version of erase. This version was in the original proposal but later was removed as a result of a bug report because it changed overload resolution for `erase(iterator)`. We provide an explicit `erase(iterator)` overload that addresses this issue. 3) less supports comparing between a smart pointer and a pointer. This allows to use flat_set<unique_ptr<T>> directly instead of doing a common workarond flat_map<T*, unique_ptr<T>>. In order to provide these features, we implement: std::enable_it_t from C++14. std::conditional_t from C++14. detection idiom (partial) from C++17. BUG=682254 ========== to ========== Improving flat containers interface. This patch adds a few features to base::flat_set/base::flat_map. 1) C++14 overloads of search operations. Now count, find, equal_range, lower_bound, upper_bound accept any type comparable with key_type if the comparison is transparent. Comparison is considered to be transparent if it has `is_transparent` member typedef (http://en.cppreference.com/w/cpp/utility/functional/less_void) We removed non-templated versions, because they were mainly for backward compatibility in extremely rare cases. 2) Templated version of erase. This version was in the original proposal but later was removed as a result of a bug report, because it changed overload resolution for `erase(iterator)`. We provide an explicit `erase(iterator)` overload that addresses this issue. 3) less supports comparing between a smart pointer and a pointer. This allows to use flat_set<unique_ptr<T>> directly instead of doing a common workaround flat_map<T*, unique_ptr<T>>. In order to provide these features, we implement: std::enable_it_t from C++14. std::conditional_t from C++14. detection idiom (partial) from C++17. BUG=682254 ==========
The CQ bit was checked by dyaroshev@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
dyaroshev@yandex-team.ru changed reviewers: + brettw@chromium.org, danakj@chromium.org, jdoerrie@chromium.org
Finally got to do this. A bit to much meta-magic but all well known easily-lookupable tricks. Thoughts?
I'm super backed up so didn't really look. But at first glance I would prefer to keep things as simple as possible and I'm not sure about the added complexity. It would be great if the other reviewers could formulate opinions about this.
On 2017/06/15 at 22:52:02, brettw wrote: > I'm super backed up so didn't really look. But at first glance I would prefer to keep things as simple as possible and I'm not sure about the added complexity. It would be great if the other reviewers could formulate opinions about this. There is a very reasonable amount of complexity for real benefits. a) C++14 interface is great, it helps to avoid constructing expensive objects. b) Comparing to smart pointers is cool, considering that the most expensive thing in flat containers is insert/erase: inserting in flat_map<int*, std::unique_ptr<int>> should be around two times more expensive than in flat_set<unique_ptr<int>>. I haven't done the measurements yet, probably will have them by next week. c) Lack of a templated erase is more of a surprise than anything, even if you can work around it with find, it's annoying. All the meta-magic is practically copy-pasted from cppreference and is higly reusable.
On 2017/06/15 at 23:23:49, dyaroshev wrote: > I haven't done the measurements yet, probably will have them by next week. Ok, so, news number 1: PauseTiming from google benchmarking library works in mysterious ways and screwed up most of my measurements that I shared previously, sorry about that. In my defence, measuring is hard. There are ways to work around this but I just inserted 500 elements in an empty set/map for this benchmark. So flat_set<std::unique_ptr> from this patch is faster by almost 35%. I think, this is useful. code: https://github.com/DenisYaroshevskiy/benchmarks/blob/master/unique_ptr_set_be... results: https://drive.google.com/open?id=0B5pZ43DhqaQDZDMwUEtwRUc5cW8
On 2017/06/16 at 19:58:52, dyaroshev wrote: > On 2017/06/15 at 23:23:49, dyaroshev wrote: > > I haven't done the measurements yet, probably will have them by next week. > > Ok, so, news number 1: > PauseTiming from google benchmarking library works in mysterious ways and screwed up most of my measurements that I shared previously, sorry about that. In my defence, measuring is hard. > > There are ways to work around this but I just inserted 500 elements in an empty set/map for this benchmark. > > So flat_set<std::unique_ptr> from this patch is faster by almost 35%. I think, this is useful. > > code: https://github.com/DenisYaroshevskiy/benchmarks/blob/master/unique_ptr_set_be... > results: https://drive.google.com/open?id=0B5pZ43DhqaQDZDMwUEtwRUc5cW8 FYI: 500 elements is where flat_map gets close to std::set on my machine. Smaller we go, bigger the gap is.
Thanks for doing this! What follows are general comments regarding this change, as I haven't done a thorough review yet. 1) I personally like the introduction of `less<void>`, and to me it makes sense to make it the default comparison, as there is hardly any backwards compatibility we would break. `less<void>` is great, because it doesn't require the construction of the `key_type` when the passed in type can be compared against it using `operator<`. This is especially great for base::DictionaryValues, where we currently need to construct temporary std::strings from base::StringPieces in order to do lookups. For calls to `Set*` this can't always be avoided, since we might add a new element to the map for which we need to construct a std::string. However, when we call `Get*`, the construction of the temporary string can be avoided as only finding a reference to the mapped Value is required. Being able to drop this temporary construction should be a significant gain and will have immediate effect, as the new API is not a requirement for this. 2) I also support the templated version of erase for the same reasons. 3) However, I am not convinced by magically supported std::unique_ptrs in base::less. You mentioned that base::flat_map<T*, std::unique_ptr<T>> is a common workaround for base::flat_set<std::unique_ptr<T>>, but I couldn't find many usages of this pattern when doing a quick code search. Furthermore, it deviates quite a bit from what the STL does, which I am not a big fan of. Lastly, given that base::flat_set can have a custom compare function, you could use base::flat_set<std::unique_ptr<T>, MyCustomCmp> if you wanted to support lookup with raw pointers, correct?
On 2017/06/18 at 05:57:53, jdoerrie wrote: > Thanks for doing this! What follows are general comments regarding this change, > as I haven't done a thorough review yet. > 2) I also support the templated version of erase for the same reasons. > FYI: you can do auto it = find(key); erase(it). Verbouse though, this is why I modified it. > 3) However, I am not convinced by magically supported std::unique_ptrs in > base::less. You mentioned that base::flat_map<T*, std::unique_ptr<T>> is a > common workaround for base::flat_set<std::unique_ptr<T>>, but I couldn't find > many usages of this pattern when doing a quick code search. Furthermore, it > deviates quite a bit from what the STL does, which I am not a big fan of. > Lastly, given that base::flat_set can have a custom compare function, you could > use base::flat_set<std::unique_ptr<T>, MyCustomCmp> if you wanted to support > lookup with raw pointers, correct? You can, but it's tricky, you have to know about is_transparent etc. I think that unique_ptr is just a pointer with a tag, unqiue doesn't have semantics to it - therefor they should be comparable. I don't insist though, if everybody feels we shouldn't do it, OK. How about a separate LessPtr?
On 2017/06/18 at 05:57:53, jdoerrie wrote: > 3) However, I am not convinced by magically supported std::unique_ptrs in > base::less. You mentioned that base::flat_map<T*, std::unique_ptr<T>> is a > common workaround for base::flat_set<std::unique_ptr<T>>, but I couldn't find > many usages of this pattern when doing a quick code search. 2 minute search. https://cs.chromium.org/chromium/src/content/browser/cache_storage/cache_stor... https://cs.chromium.org/chromium/src/components/precache/core/fetcher_pool.h?... https://cs.chromium.org/chromium/src/components/domain_reliability/uploader.c... https://cs.chromium.org/chromium/src/net/http/http_network_session.h?l=331&rc... https://cs.chromium.org/chromium/src/net/reporting/reporting_cache.cc?l=290&r... https://cs.chromium.org/chromium/src/net/proxy/mojo_proxy_resolver_impl.h?l=3... You probaby didn't include all maps, only flat. My measurements show that flat_set with one pointer is almost two times faster (on my machine) than std::map with two for a huge map with 500 elements (smaller - bigger gap) for inserting by one, which is a bad case.
On 2017/06/18 at 05:57:53, jdoerrie wrote: > 3) However, I am not convinced by magically supported std::unique_ptrs in > base::less. You mentioned that base::flat_map<T*, std::unique_ptr<T>> is a > common workaround for base::flat_set<std::unique_ptr<T>>, but I couldn't find > many usages of this pattern when doing a quick code search. 2 minute search. https://cs.chromium.org/chromium/src/content/browser/cache_storage/cache_stor... https://cs.chromium.org/chromium/src/components/precache/core/fetcher_pool.h?... https://cs.chromium.org/chromium/src/components/domain_reliability/uploader.c... https://cs.chromium.org/chromium/src/net/http/http_network_session.h?l=331&rc... https://cs.chromium.org/chromium/src/net/reporting/reporting_cache.cc?l=290&r... https://cs.chromium.org/chromium/src/net/proxy/mojo_proxy_resolver_impl.h?l=3... You probaby didn't include all maps, only flat. My measurements show that flat_set with one pointer is almost two times faster (on my machine) than std::map with two for a huge map with 500 elements (smaller - bigger gap) for inserting by one, which is a bad case.
On 2017/06/18 14:43:29, dyaroshev wrote: > On 2017/06/18 at 05:57:53, jdoerrie wrote: > > 3) However, I am not convinced by magically supported std::unique_ptrs in > > base::less. You mentioned that base::flat_map<T*, std::unique_ptr<T>> is a > > common workaround for base::flat_set<std::unique_ptr<T>>, but I couldn't find > > many usages of this pattern when doing a quick code search. > > 2 minute search. > > https://cs.chromium.org/chromium/src/content/browser/cache_storage/cache_stor... > https://cs.chromium.org/chromium/src/components/precache/core/fetcher_pool.h?... > https://cs.chromium.org/chromium/src/components/domain_reliability/uploader.c... > https://cs.chromium.org/chromium/src/net/http/http_network_session.h?l=331&rc... > https://cs.chromium.org/chromium/src/net/reporting/reporting_cache.cc?l=290&r... > https://cs.chromium.org/chromium/src/net/proxy/mojo_proxy_resolver_impl.h?l=3... > > You probaby didn't include all maps, only flat. My measurements show that > flat_set with one pointer is almost two times faster (on my machine) than > std::map with two for a huge map with 500 elements (smaller - bigger gap) for > inserting by one, which is a bad case. Fair point, you are right, I did only look for the flat_* variants. Here's is an SO thread dealing with the same issue: https://stackoverflow.com/q/18939882 I would be happy with a separate base::LessPtr to deal with this, but I'm also curious what the other reviewers think about this.
On 2017/06/19 at 20:12:57, jdoerrie wrote: > Fair point, you are right, I did only look for the flat_* variants. Here's is an SO thread dealing with the same issue: https://stackoverflow.com/q/18939882 > > I would be happy with a separate base::LessPtr to deal with this, but I'm also curious what the other reviewers think about this. I still think that my approach is reasonable, it makes sense for me to compare smart and regular pointers. But I see how other people might not agree with me.
I'm supportive of adding C++14 templatized lookup support to this class. This will indeed solve some problems, especially for base::Value! I'm not currently convinced of the need for the pointer comparison thing. Can you remove that from this CL and any associated template stuff? I want to stress that we really want to be minimal about what we add to base. We should only be adding things we really need, rather than building a C++17 transition library or something. If there are features from C++14 or C++17 we need, I'm OK adding them to base. We should be clear what these things are. I'm thinking about type_traits.h where I think each declaration should say what std::thing it's an implementation of (or that it's an implementation detail of something else), and also mention that it can be removed when we move to some newer version of C++. If something is partially supported, can it be explicit about how it diverges from the standard? I think with these changes I'll have a much easier time giving a good opinion about the additions. Thanks! Sorry I'm so delayed. https://codereview.chromium.org/2944523002/diff/20001/base/containers/flat_map.h File base/containers/flat_map.h (right): https://codereview.chromium.org/2944523002/diff/20001/base/containers/flat_ma... base/containers/flat_map.h:106: // template <typename ...Args> I find these ...Args template lines make it more difficult to follow which is why I left them out of this quick-reference. It seems obvious enough what's going on without them. https://codereview.chromium.org/2944523002/diff/20001/base/containers/flat_ma... base/containers/flat_map.h:116: // size_t erase(const K& key) Here and in the ones below we really need the template definition or it makes no sense, but I think they can all fit on one line. That will solve the weird ambiguity of where lines begin and end and make it easy to scan down as a reference. (These two comments also apply to the other references.) https://codereview.chromium.org/2944523002/diff/20001/base/containers/flat_tr... File base/containers/flat_tree.h (right): https://codereview.chromium.org/2944523002/diff/20001/base/containers/flat_tr... base/containers/flat_tree.h:911: // if the compare is not transparent -> construct key_type once. Comment nits: capital "I" (same below)
On 2017/06/20 at 18:13:35, brettw wrote: > I'm supportive of adding C++14 templatized lookup support to this class. This will indeed solve some problems, especially for base::Value! > > I'm not currently convinced of the need for the pointer comparison thing. Can you remove that from this CL and any associated template stuff? I want to stress that we really want to be minimal about what we add to base. We should only be adding things we really need, rather than building a C++17 transition library or something. > > If there are features from C++14 or C++17 we need, I'm OK adding them to base. We should be clear what these things are. I'm thinking about type_traits.h where I think each declaration should say what std::thing it's an implementation of (or that it's an implementation detail of something else), and also mention that it can be removed when we move to some newer version of C++. If something is partially supported, can it be explicit about how it diverges from the standard? > > I think with these changes I'll have a much easier time giving a good opinion about the additions. Thanks! Sorry I'm so delayed. > > https://codereview.chromium.org/2944523002/diff/20001/base/containers/flat_map.h > File base/containers/flat_map.h (right): > > https://codereview.chromium.org/2944523002/diff/20001/base/containers/flat_ma... > base/containers/flat_map.h:106: // template <typename ...Args> > I find these ...Args template lines make it more difficult to follow which is why I left them out of this quick-reference. It seems obvious enough what's going on without them. > > https://codereview.chromium.org/2944523002/diff/20001/base/containers/flat_ma... > base/containers/flat_map.h:116: // size_t erase(const K& key) > Here and in the ones below we really need the template definition or it makes no sense, but I think they can all fit on one line. That will solve the weird ambiguity of where lines begin and end and make it easy to scan down as a reference. > > (These two comments also apply to the other references.) > > https://codereview.chromium.org/2944523002/diff/20001/base/containers/flat_tr... > File base/containers/flat_tree.h (right): > > https://codereview.chromium.org/2944523002/diff/20001/base/containers/flat_tr... > base/containers/flat_tree.h:911: // if the compare is not transparent -> construct key_type once. > Comment nits: capital "I" (same below) Done?
LGTM % nits https://codereview.chromium.org/2944523002/diff/40001/base/containers/README.md File base/containers/README.md (right): https://codereview.chromium.org/2944523002/diff/40001/base/containers/README.... base/containers/README.md:49: where you make many objects so that the code/heap tradeoff is good. nit: typo https://codereview.chromium.org/2944523002/diff/40001/base/containers/flat_tr... File base/containers/flat_tree.h (right): https://codereview.chromium.org/2944523002/diff/40001/base/containers/flat_tr... base/containers/flat_tree.h:356: return extractor(v); Just out of curiosity: Why don't we simply inline extractor here and in other places? Wouldn't `return GetKeyFromValue()(v);` also work? https://codereview.chromium.org/2944523002/diff/40001/base/containers/flat_tr... base/containers/flat_tree.h:916: return std::lower_bound(begin(), end(), key_ref, key_value); key_value could also be inlined, correct? `KeyValueCompare(key_comp())` should be the same. https://codereview.chromium.org/2944523002/diff/40001/base/containers/flat_tr... File base/containers/flat_tree_unittest.cc (right): https://codereview.chromium.org/2944523002/diff/40001/base/containers/flat_tr... base/containers/flat_tree_unittest.cc:22: // if key has a templated constructor. We have to fix this. Is this comment still relevant? https://codereview.chromium.org/2944523002/diff/40001/base/type_traits.h File base/type_traits.h (right): https://codereview.chromium.org/2944523002/diff/40001/base/type_traits.h#newc... base/type_traits.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. base/template_util.h (https://codesearch.chromium.org/chromium/src/base/template_util.h) already tries to provide some missing std type_traits that are not present on old GCC versions. With the existence of this file these two files should probably be merged, however doing this in a follow-up CL seems very reasonable to me.
Hey, what's going on with codereview?) Is it April Fools or smth?) https://codereview.chromium.org/2944523002/diff/40001/base/containers/flat_tr... File base/containers/flat_tree.h (right): https://codereview.chromium.org/2944523002/diff/40001/base/containers/flat_tr... base/containers/flat_tree.h:356: return extractor(v); On 2017/06/20 at 23:51:09, jdoerrie wrote: > Just out of curiosity: Why don't we simply inline extractor here and in other places? Wouldn't `return GetKeyFromValue()(v);` also work? Sure. I just kept it as is. https://codereview.chromium.org/2944523002/diff/40001/base/containers/flat_tr... base/containers/flat_tree.h:916: return std::lower_bound(begin(), end(), key_ref, key_value); On 2017/06/20 at 23:51:09, jdoerrie wrote: > key_value could also be inlined, correct? `KeyValueCompare(key_comp())` should be the same. Yep, but previously it wasn't so I didn't touch it. https://codereview.chromium.org/2944523002/diff/40001/base/containers/flat_tr... File base/containers/flat_tree_unittest.cc (right): https://codereview.chromium.org/2944523002/diff/40001/base/containers/flat_tr... base/containers/flat_tree_unittest.cc:22: // if key has a templated constructor. We have to fix this. On 2017/06/20 at 23:51:09, jdoerrie wrote: > Is this comment still relevant? Thanks. Added the test. https://codereview.chromium.org/2944523002/diff/40001/base/type_traits.h File base/type_traits.h (right): https://codereview.chromium.org/2944523002/diff/40001/base/type_traits.h#newc... base/type_traits.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/06/20 at 23:51:09, jdoerrie wrote: > base/template_util.h (https://codesearch.chromium.org/chromium/src/base/template_util.h) already tries to provide some missing std type_traits that are not present on old GCC versions. With the existence of this file these two files should probably be merged, however doing this in a follow-up CL seems very reasonable to me. Done - merged it here. Also replaced local implementation of the detection idiom with the one from the future standard.
Description was changed from ========== Improving flat containers interface. This patch adds a few features to base::flat_set/base::flat_map. 1) C++14 overloads of search operations. Now count, find, equal_range, lower_bound, upper_bound accept any type comparable with key_type if the comparison is transparent. Comparison is considered to be transparent if it has `is_transparent` member typedef (http://en.cppreference.com/w/cpp/utility/functional/less_void) We removed non-templated versions, because they were mainly for backward compatibility in extremely rare cases. 2) Templated version of erase. This version was in the original proposal but later was removed as a result of a bug report, because it changed overload resolution for `erase(iterator)`. We provide an explicit `erase(iterator)` overload that addresses this issue. 3) less supports comparing between a smart pointer and a pointer. This allows to use flat_set<unique_ptr<T>> directly instead of doing a common workaround flat_map<T*, unique_ptr<T>>. In order to provide these features, we implement: std::enable_it_t from C++14. std::conditional_t from C++14. detection idiom (partial) from C++17. BUG=682254 ========== to ========== Improving flat containers interface. This patch adds a few features to base::flat_set/base::flat_map. 1) C++14 overloads of search operations. Now count, find, equal_range, lower_bound, upper_bound accept any type comparable with key_type if the comparison is transparent. Comparison is considered to be transparent if it has `is_transparent` member typedef (http://en.cppreference.com/w/cpp/utility/functional/less_void) We removed non-templated versions, because they were mainly for backward compatibility in extremely rare cases. 2) Templated version of erase. This version was in the original proposal but later was removed as a result of a bug report, because it changed overload resolution for `erase(iterator)`. We provide an explicit `erase(iterator)` overload that addresses this issue. In order to provide these features, we implement: std::enable_it_t from C++14. std::conditional_t from C++14. detection idiom (partial) from C++17. BUG=682254 ==========
The CQ bit was checked by dyaroshev@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Sorry, I was on a 59 release blocker and not really looking at anything else :( Hopefully fixed now so I should be a little better.
Hey, sorry to be slow. These newer c++ concepts are quite new to me. I'm adding vmpstr to this as well as resident c++ expert. We had a bit of a convo about this and I'd like him to guide this to a place he's happy with. I'm excited about not constructing temp strings to do queries, but not super sure about adding so much experimental stuff in base, and would like him to sort thru things. https://codereview.chromium.org/2944523002/diff/60001/base/template_util.h File base/template_util.h (right): https://codereview.chromium.org/2944523002/diff/60001/base/template_util.h#ne... base/template_util.h:39: // C++14 --------------------------------------------------------------------- I would honestly prefer to not add things here when you can already do it with the STL as we have it, and would have code use enable_if::type directly instead.
The only thing I'm unsure about are how much of the additions to template_util we need. Like Dana, I would like to keep such things to a minimum, but I also don't understand how much this gets us. If calling code is way more clear it may be worthwhile to consider. https://codereview.chromium.org/2944523002/diff/60001/base/containers/flat_map.h File base/containers/flat_map.h (right): https://codereview.chromium.org/2944523002/diff/60001/base/containers/flat_ma... base/containers/flat_map.h:139: template <class Key, class Mapped, class Compare = less> I would feel slightly better if "less" was fully qualified since it's easy to get confused about which one this is referring to. So "::base::less"? https://codereview.chromium.org/2944523002/diff/60001/base/containers/flat_set.h File base/containers/flat_set.h (right): https://codereview.chromium.org/2944523002/diff/60001/base/containers/flat_se... base/containers/flat_set.h:123: template <class Key, class Compare = less> Ditto on fully qualified. https://codereview.chromium.org/2944523002/diff/60001/base/template_util.h File base/template_util.h (right): https://codereview.chromium.org/2944523002/diff/60001/base/template_util.h#ne... base/template_util.h:39: // C++14 --------------------------------------------------------------------- I think all this is needed is for TransparentCompare. Is that correct? What does TransparectCompare look like if we don't add this stuff?
vmpstr@chromium.org changed reviewers: + vmpstr@chromium.org
https://codereview.chromium.org/2944523002/diff/60001/base/containers/README.md File base/containers/README.md (right): https://codereview.chromium.org/2944523002/diff/60001/base/containers/README.... base/containers/README.md:127: flat\_set/flat\_map support C++14 interface and a notion of transparent Can you link to documentation about transparent comparators in the comment? https://codereview.chromium.org/2944523002/diff/60001/base/containers/flat_se... File base/containers/flat_set_unittest.cc (right): https://codereview.chromium.org/2944523002/diff/60001/base/containers/flat_se... base/containers/flat_set_unittest.cc:98: s.count(x); Can you leave a comment that this is checking if things compile and what exactly it's checking https://codereview.chromium.org/2944523002/diff/60001/base/containers/flat_tr... File base/containers/flat_tree.h (right): https://codereview.chromium.org/2944523002/diff/60001/base/containers/flat_tr... base/containers/flat_tree.h:62: template <typename Y> T? https://codereview.chromium.org/2944523002/diff/60001/base/containers/flat_tr... base/containers/flat_tree.h:912: conditional_t<TransparentCompare<key_compare>(), const K&, const key_type&> I think just const std::conditional<TransparentCompare<key_compare>(), K, key_type>::type& key_ref would be better? https://codereview.chromium.org/2944523002/diff/60001/base/template_util.h File base/template_util.h (right): https://codereview.chromium.org/2944523002/diff/60001/base/template_util.h#ne... base/template_util.h:39: // C++14 --------------------------------------------------------------------- On 2017/06/23 21:10:26, danakj wrote: > I would honestly prefer to not add things here when you can already do it with > the STL as we have it, and would have code use enable_if::type directly instead. I agree. I'd rather not start adding things from C++14 onward if they are just syntactic sugar. Eventually we'll move to C++14 proper and be able to use those then. It's hard enough to keep remembering when to type std:: and when to type base:: outside of //base. When C++14 arrives, and we remove these, then the code outside doesn't have to change to compile but could optionally be updated for std::enable_if_t and the like. https://codereview.chromium.org/2944523002/diff/60001/base/template_util.h#ne... base/template_util.h:51: // These can be replaced with the ones from the standard once we have C++17. I'll admit I'm not too familiar with the detection idiom that you have here. I'll read up more on it, but is based on the SupportsOstreamOperator changes below, it seems that this is already doable succinctly with the current version of C++. Is that not true? (I mean to say we only need to know about transparency of comparators, and not a general detection logic, right?) Similarly to above, I'd prefer to have fewer future language version implemented in base, especially from std::experimental since that's definitely subject to change. https://codereview.chromium.org/2944523002/diff/60001/base/template_util.h#ne... base/template_util.h:91: typename internal::detector<nonesuch, void, Op, Args...>::value_t; AFAICT, we're not using detector::type, just the value_t, so I think we could at least clean this up by eliminating nonesuch and type parameters. We only need void_t since we need to determine whether Op<Args...> is a valid type. Is that true? I understand that it's parity with c++17, but all of this is experimental. https://codereview.chromium.org/2944523002/diff/60001/base/template_util.h#ne... base/template_util.h:98: return is_detected<Op, Args...>::value; Can you comment that ::value is on std::true_type/std::false_type, it takes a bit of cognitive processing to figure out what's coming from where
https://codereview.chromium.org/2944523002/diff/60001/base/containers/README.md File base/containers/README.md (right): https://codereview.chromium.org/2944523002/diff/60001/base/containers/README.... base/containers/README.md:127: flat\_set/flat\_map support C++14 interface and a notion of transparent On 2017/06/23 at 21:45:14, vmpstr wrote: > Can you link to documentation about transparent comparators in the comment? Sorry - missed this. Will do with next patch. Do docks for std::less work for you? http://en.cppreference.com/w/cpp/utility/functional/less_void https://codereview.chromium.org/2944523002/diff/60001/base/containers/flat_map.h File base/containers/flat_map.h (right): https://codereview.chromium.org/2944523002/diff/60001/base/containers/flat_ma... base/containers/flat_map.h:139: template <class Key, class Mapped, class Compare = less> On 2017/06/23 at 21:19:45, brettw wrote: > I would feel slightly better if "less" was fully qualified since it's easy to get confused about which one this is referring to. So "::base::less"? Done https://codereview.chromium.org/2944523002/diff/60001/base/containers/flat_set.h File base/containers/flat_set.h (right): https://codereview.chromium.org/2944523002/diff/60001/base/containers/flat_se... base/containers/flat_set.h:123: template <class Key, class Compare = less> On 2017/06/23 at 21:19:45, brettw wrote: > Ditto on fully qualified. Done https://codereview.chromium.org/2944523002/diff/60001/base/containers/flat_tr... File base/containers/flat_tree.h (right): https://codereview.chromium.org/2944523002/diff/60001/base/containers/flat_tr... base/containers/flat_tree.h:62: template <typename Y> On 2017/06/23 at 21:45:14, vmpstr wrote: > T? Not applicable. https://codereview.chromium.org/2944523002/diff/60001/base/containers/flat_tr... base/containers/flat_tree.h:912: conditional_t<TransparentCompare<key_compare>(), const K&, const key_type&> On 2017/06/23 at 21:45:14, vmpstr wrote: > I think just > const std::conditional<TransparentCompare<key_compare>(), K, key_type>::type& key_ref > > would be better? You forgot "typename". Anyways, not applicable anymore. https://codereview.chromium.org/2944523002/diff/60001/base/template_util.h File base/template_util.h (right): https://codereview.chromium.org/2944523002/diff/60001/base/template_util.h#ne... base/template_util.h:39: // C++14 --------------------------------------------------------------------- On 2017/06/23 at 21:45:14, vmpstr wrote: > On 2017/06/23 21:10:26, danakj wrote: > > I would honestly prefer to not add things here when you can already do it with > > the STL as we have it, and would have code use enable_if::type directly instead. > > I agree. I'd rather not start adding things from C++14 onward if they are just syntactic sugar. Eventually we'll move to C++14 proper and be able to use those then. It's hard enough to keep remembering when to type std:: and when to type base:: outside of //base. When C++14 arrives, and we remove these, then the code outside doesn't have to change to compile but could optionally be updated for std::enable_if_t and the like. This is why we can't have nice things) Ok, I found another implementation of void_t in the codebase and used it for Transparent Check. https://codereview.chromium.org/2944523002/diff/60001/base/template_util.h#ne... base/template_util.h:51: // These can be replaced with the ones from the standard once we have C++17. On 2017/06/23 at 21:45:14, vmpstr wrote: > I'll admit I'm not too familiar with the detection idiom that you have here. I'll read up more on it, but is based on the SupportsOstreamOperator changes below, it seems that this is already doable succinctly with the current version of C++. Is that not true? (I mean to say we only need to know about transparency of comparators, and not a general detection logic, right?) > > Similarly to above, I'd prefer to have fewer future language version implemented in base, especially from std::experimental since that's definitely subject to change. Done. https://codereview.chromium.org/2944523002/diff/60001/base/template_util.h#ne... base/template_util.h:91: typename internal::detector<nonesuch, void, Op, Args...>::value_t; On 2017/06/23 at 21:45:14, vmpstr wrote: > AFAICT, we're not using detector::type, just the value_t, so I think we could at least clean this up by eliminating nonesuch and type parameters. We only need void_t since we need to determine whether Op<Args...> is a valid type. Is that true? > > I understand that it's parity with c++17, but all of this is experimental. Removed altogether. https://codereview.chromium.org/2944523002/diff/60001/base/template_util.h#ne... base/template_util.h:98: return is_detected<Op, Args...>::value; On 2017/06/23 at 21:45:14, vmpstr wrote: > Can you comment that ::value is on std::true_type/std::false_type, it takes a bit of cognitive processing to figure out what's coming from where Not applicable.
This is looking really good, just nits below https://codereview.chromium.org/2944523002/diff/60001/base/containers/README.md File base/containers/README.md (right): https://codereview.chromium.org/2944523002/diff/60001/base/containers/README.... base/containers/README.md:127: flat\_set/flat\_map support C++14 interface and a notion of transparent On 2017/06/26 11:47:29, dyaroshev wrote: > On 2017/06/23 at 21:45:14, vmpstr wrote: > > Can you link to documentation about transparent comparators in the comment? > > Sorry - missed this. Will do with next patch. Do docks for std::less work for > you? http://en.cppreference.com/w/cpp/utility/functional/less_void Yeah that works; just something for people to read up on if interested in transparent comparisons https://codereview.chromium.org/2944523002/diff/60001/base/template_util.h File base/template_util.h (right): https://codereview.chromium.org/2944523002/diff/60001/base/template_util.h#ne... base/template_util.h:39: // C++14 --------------------------------------------------------------------- On 2017/06/26 11:47:29, dyaroshev wrote: > On 2017/06/23 at 21:45:14, vmpstr wrote: > > On 2017/06/23 21:10:26, danakj wrote: > > > I would honestly prefer to not add things here when you can already do it > with > > > the STL as we have it, and would have code use enable_if::type directly > instead. > > > > I agree. I'd rather not start adding things from C++14 onward if they are just > syntactic sugar. Eventually we'll move to C++14 proper and be able to use those > then. It's hard enough to keep remembering when to type std:: and when to type > base:: outside of //base. When C++14 arrives, and we remove these, then the code > outside doesn't have to change to compile but could optionally be updated for > std::enable_if_t and the like. > > This is why we can't have nice things) Indeed > > Ok, I found another implementation of void_t in the codebase and used it for > Transparent Check. https://codereview.chromium.org/2944523002/diff/80001/base/containers/flat_tr... File base/containers/flat_tree.h (right): https://codereview.chromium.org/2944523002/diff/80001/base/containers/flat_tr... base/containers/flat_tree.h:477: using PickConstReferenceTypeT = nit: Maybe KeyTypeOrT? I'd still weakly prefer this to be either K or key_type in the conditional and const typename KeyTypeOrT<K>::type& key_ref in the calling code. It just makes the const and the & part be more local to the caller. It's just a preference though, so feel free to keep it as is. If you do, then I guess the name is OK :) https://codereview.chromium.org/2944523002/diff/80001/base/containers/flat_tr... base/containers/flat_tree.h:915: // Only compile when conversion is implicit. I'm not sure what this comment is referring to. This will only compile if K -> key_type conversion is implicit? Maybe just say "pick K if the comparator is transparent, and key_type otherwise"? https://codereview.chromium.org/2944523002/diff/80001/base/containers/flat_tr... File base/containers/flat_tree_unittest.cc (right): https://codereview.chromium.org/2944523002/diff/80001/base/containers/flat_tr... base/containers/flat_tree_unittest.cc:150: flat_tree<int, int, GetKeyFromValueIdentity<int>, std::less<int>>; Since the default is base::less, should we make it that here, so that the tests mostly have the default version tested? https://codereview.chromium.org/2944523002/diff/80001/base/functional.h File base/functional.h (right): https://codereview.chromium.org/2944523002/diff/80001/base/functional.h#newco... base/functional.h:18: constexpr auto operator()(T&& lhs, U&& rhs) const "bool operator(...) const", since std::less::operator() returns bool. Or are there specific cases that you want handled here that otherwise wouldn't be?
https://codereview.chromium.org/2944523002/diff/80001/base/containers/flat_tr... File base/containers/flat_tree.h (right): https://codereview.chromium.org/2944523002/diff/80001/base/containers/flat_tr... base/containers/flat_tree.h:477: using PickConstReferenceTypeT = On 2017/06/26 at 15:35:53, vmpstr wrote: > nit: Maybe KeyTypeOrT? I'd still weakly prefer this to be either K or key_type in the conditional and > > const typename KeyTypeOrT<K>::type& key_ref > > in the calling code. It just makes the const and the & part be more local to the caller. It's just a preference though, so feel free to keep it as is. If you do, then I guess the name is OK :) Done. https://codereview.chromium.org/2944523002/diff/80001/base/containers/flat_tr... base/containers/flat_tree.h:915: // Only compile when conversion is implicit. On 2017/06/26 at 15:35:53, vmpstr wrote: > I'm not sure what this comment is referring to. This will only compile if K -> key_type conversion is implicit? > > Maybe just say "pick K if the comparator is transparent, and key_type otherwise"? This is about using assignment over operator(). This way, if you type: ``` base::flat_set<std::unique_ptr<int>, std::less<std::unique_ptr<int>>> test; test.lower_bound(&x); ``` You get a compilation error: ``` ../../base/containers/flat_tree.h:916:24: error: reference to type 'const KeyTypeOrK<int *>' (aka 'const std::__1::unique_ptr<int, std::__1::default_delete<int> >') could not bind to an lvalue of type 'int *const' const KeyTypeOrK<K>& key_ref = key; ``` If you instead use uniform initialisation, for example, it will compile and crash. https://codereview.chromium.org/2944523002/diff/80001/base/containers/flat_tr... File base/containers/flat_tree_unittest.cc (right): https://codereview.chromium.org/2944523002/diff/80001/base/containers/flat_tr... base/containers/flat_tree_unittest.cc:150: flat_tree<int, int, GetKeyFromValueIdentity<int>, std::less<int>>; On 2017/06/26 at 15:35:53, vmpstr wrote: > Since the default is base::less, should we make it that here, so that the tests mostly have the default version tested? I thought about that and decided not to - I need to check both cases anyways, for one of them it's enough just for compilation and the other one would check correctness. They still test same methods though, I don't see why to switch. https://codereview.chromium.org/2944523002/diff/80001/base/functional.h File base/functional.h (right): https://codereview.chromium.org/2944523002/diff/80001/base/functional.h#newco... base/functional.h:18: constexpr auto operator()(T&& lhs, U&& rhs) const On 2017/06/26 at 15:35:53, vmpstr wrote: > "bool operator(...) const", since std::less::operator() returns bool. Or are there specific cases that you want handled here that otherwise wouldn't be? This is how std::less is defined http://en.cppreference.com/w/cpp/utility/functional/less_void I don't really know why, but they probably had a reason.
lgtm. I think you also need to update the patch description to reflect the recent changes. https://codereview.chromium.org/2944523002/diff/80001/base/containers/flat_tr... File base/containers/flat_tree_unittest.cc (right): https://codereview.chromium.org/2944523002/diff/80001/base/containers/flat_tr... base/containers/flat_tree_unittest.cc:150: flat_tree<int, int, GetKeyFromValueIdentity<int>, std::less<int>>; On 2017/06/26 16:19:28, dyaroshev wrote: > On 2017/06/26 at 15:35:53, vmpstr wrote: > > Since the default is base::less, should we make it that here, so that the > tests mostly have the default version tested? > > I thought about that and decided not to - I need to check both cases anyways, > for one of them it's enough just for compilation and the other one would check > correctness. They still test same methods though, I don't see why to switch. Acknowledged. https://codereview.chromium.org/2944523002/diff/80001/base/functional.h File base/functional.h (right): https://codereview.chromium.org/2944523002/diff/80001/base/functional.h#newco... base/functional.h:18: constexpr auto operator()(T&& lhs, U&& rhs) const On 2017/06/26 16:19:29, dyaroshev wrote: > On 2017/06/26 at 15:35:53, vmpstr wrote: > > "bool operator(...) const", since std::less::operator() returns bool. Or are > there specific cases that you want handled here that otherwise wouldn't be? > > This is how std::less is defined > http://en.cppreference.com/w/cpp/utility/functional/less_void > I don't really know why, but they probably had a reason. Oh interesting. I was just looking at general std::less, not the void specialization. Can you leave that link in the comment on line 13?
What happens if you have something like flat_set<int> and then do a find(1.5f); on it? Does that compile and work? I believe some components have narrowing conversion warnings enabled, but if it's transparent would that just compare an int to a float?
On 2017/06/26 at 19:05:54, vmpstr wrote: > What happens if you have something like flat_set<int> and then do a find(1.5f); on it? Does that compile and work? > > I believe some components have narrowing conversion warnings enabled, but if it's transparent would that just compare an int to a float? I checked in base_unittests with clang, you are correct - compiles and runs. I recently got a waring from MSVS, maybe it will catch such a problem? I don't know, do you have something in mind for this issue?
Description was changed from ========== Improving flat containers interface. This patch adds a few features to base::flat_set/base::flat_map. 1) C++14 overloads of search operations. Now count, find, equal_range, lower_bound, upper_bound accept any type comparable with key_type if the comparison is transparent. Comparison is considered to be transparent if it has `is_transparent` member typedef (http://en.cppreference.com/w/cpp/utility/functional/less_void) We removed non-templated versions, because they were mainly for backward compatibility in extremely rare cases. 2) Templated version of erase. This version was in the original proposal but later was removed as a result of a bug report, because it changed overload resolution for `erase(iterator)`. We provide an explicit `erase(iterator)` overload that addresses this issue. In order to provide these features, we implement: std::enable_it_t from C++14. std::conditional_t from C++14. detection idiom (partial) from C++17. BUG=682254 ========== to ========== Improving flat containers interface. This patch adds a few features to base::flat_set/base::flat_map. 1) C++14 overloads of search operations. Now count, find, equal_range, lower_bound, upper_bound accept any type comparable with key_type if the comparison is transparent. Comparison is considered to be transparent if it has `is_transparent` member typedef (http://en.cppreference.com/w/cpp/utility/functional/less_void) We removed non-templated versions, because they were mainly for backward compatibility in extremely rare cases. 2) Templated version of erase. This version was in the original proposal but later was removed as a result of a bug report, because it changed overload resolution for `erase(iterator)`. We provide an explicit `erase(iterator)` overload that addresses this issue. BUG=682254 ==========
On Mon, Jun 26, 2017 at 3:10 PM, <dyaroshev@yandex-team.ru> wrote: > On 2017/06/26 at 19:05:54, vmpstr wrote: > > What happens if you have something like flat_set<int> and then do a > find(1.5f); on it? Does that compile and work? > > > > I believe some components have narrowing conversion warnings enabled, > but if > it's transparent would that just compare an int to a float? > > I checked in base_unittests with clang, you are correct - compiles and > runs. I > recently got a waring from MSVS, maybe it will catch such a problem? > > I don't know, do you have something in mind for this issue? > Is there any existing code that depends on this behaviour? Can you make it not compile to find it with bots? If not then we can change it, if there is we need to consider them/fix them. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by dyaroshev@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/26 at 19:13:26, danakj wrote: > On Mon, Jun 26, 2017 at 3:10 PM, <dyaroshev@yandex-team.ru> wrote: > > > On 2017/06/26 at 19:05:54, vmpstr wrote: > > > What happens if you have something like flat_set<int> and then do a > > find(1.5f); on it? Does that compile and work? > > > > > > I believe some components have narrowing conversion warnings enabled, > > but if > > it's transparent would that just compare an int to a float? > > > > I checked in base_unittests with clang, you are correct - compiles and > > runs. I > > recently got a waring from MSVS, maybe it will catch such a problem? > > > > I don't know, do you have something in mind for this issue? > > > > Is there any existing code that depends on this behaviour? Can you make it > not compile to find it with bots? If not then we can change it, if there is > we need to consider them/fix them. > I started bot's run, but I don't exactly see in what's way. Do you mean than before this patch looking up 1.5f in a set {1, 2} would return 1, and now two? Interesting, crasy a bit though.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Thanks for reviewing vmpstr! I have a few base structure comments mostly. https://codereview.chromium.org/2944523002/diff/100001/base/containers/README.md File base/containers/README.md (right): https://codereview.chromium.org/2944523002/diff/100001/base/containers/README... base/containers/README.md:127: flat\_set/flat\_map support C++14 interface and a notion of transparent Can you add a code example here of using flat_map or flat_set with unique_ptr and with std::string? And once this lands, also send an email to chromium-dev including those examples in it? https://codereview.chromium.org/2944523002/diff/100001/base/containers/flat_m... File base/containers/flat_map_unittest.cc (right): https://codereview.chromium.org/2944523002/diff/100001/base/containers/flat_m... base/containers/flat_map_unittest.cc:173: TEST(FlatMap, UniquePtrKeys) { nit: there's no UniquePtrs in this test, CompareWithUnderlyingType or something? https://codereview.chromium.org/2944523002/diff/100001/base/containers/flat_s... File base/containers/flat_set_unittest.cc (right): https://codereview.chromium.org/2944523002/diff/100001/base/containers/flat_s... base/containers/flat_set_unittest.cc:93: TEST(FlatSet, UniquePtrs) { same nit https://codereview.chromium.org/2944523002/diff/100001/base/containers/flat_t... File base/containers/flat_tree.h (right): https://codereview.chromium.org/2944523002/diff/100001/base/containers/flat_t... base/containers/flat_tree.h:913: // Only compile when conversion is implicit. What does the compiler error look like when this fails? https://codereview.chromium.org/2944523002/diff/100001/base/functional.h File base/functional.h (right): https://codereview.chromium.org/2944523002/diff/100001/base/functional.h#newc... base/functional.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. Can this go in template_util.h? We haven't reproduced the STL header structure in base and I'm not sure that we want to start now. https://codereview.chromium.org/2944523002/diff/100001/base/functional.h#newc... base/functional.h:14: // Needed for improved flat containers API. remove this line https://codereview.chromium.org/2944523002/diff/100001/base/functional.h#newc... base/functional.h:23: using is_transparent = int; Refer to transparent docs here also please https://codereview.chromium.org/2944523002/diff/100001/base/functional.h#newc... base/functional.h:26: } // base namespace base https://codereview.chromium.org/2944523002/diff/100001/base/functional_unitte... File base/functional_unittest.cc (right): https://codereview.chromium.org/2944523002/diff/100001/base/functional_unitte... base/functional_unittest.cc:7: #include "base/containers/container_test_utils.h" this file should be in base/test/, especially if it's going to be exporting types. really as base/test/move_only_int.h https://codereview.chromium.org/2944523002/diff/100001/base/functional_unitte... base/functional_unittest.cc:12: nit: remove whitespace https://codereview.chromium.org/2944523002/diff/100001/base/functional_unitte... base/functional_unittest.cc:25: nit: remove whitespace
On Mon, Jun 26, 2017 at 3:16 PM, <dyaroshev@yandex-team.ru> wrote: > On 2017/06/26 at 19:13:26, danakj wrote: > > On Mon, Jun 26, 2017 at 3:10 PM, <dyaroshev@yandex-team.ru> wrote: > > > > > On 2017/06/26 at 19:05:54, vmpstr wrote: > > > > What happens if you have something like flat_set<int> and then do a > > > find(1.5f); on it? Does that compile and work? > > > > > > > > I believe some components have narrowing conversion warnings enabled, > > > but if > > > it's transparent would that just compare an int to a float? > > > > > > I checked in base_unittests with clang, you are correct - compiles and > > > runs. I > > > recently got a waring from MSVS, maybe it will catch such a problem? > > > > > > I don't know, do you have something in mind for this issue? > > > > > > > Is there any existing code that depends on this behaviour? Can you make > it > > not compile to find it with bots? If not then we can change it, if there > is > > we need to consider them/fix them. > > > > I started bot's run, but I don't exactly see in what's way. Do you mean > than > before this patch looking up 1.5f in a set {1, 2} would return 1, and now > two? > Interesting, crasy a bit though. > Thanks! find(1.5) would find 1 before, whereas now find(1.5) would give you end(), right? It's a bit crazy, but let's make sure, chromium is huge and you never know what happens. Someone may be adding floats and then doing lookup for some reason... > > > https://codereview.chromium.org/2944523002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/06/26 19:10:58, dyaroshev wrote: > On 2017/06/26 at 19:05:54, vmpstr wrote: > > What happens if you have something like flat_set<int> and then do a > find(1.5f); on it? Does that compile and work? > > > > I believe some components have narrowing conversion warnings enabled, but if > it's transparent would that just compare an int to a float? > > I checked in base_unittests with clang, you are correct - compiles and runs. I > recently got a waring from MSVS, maybe it will catch such a problem? > > I don't know, do you have something in mind for this issue? I don't really have a simple solution here, it's just one of those gotchas... FWIW I am still for doing this and treating these cases as user errors at most.
I'm hitting some weird issue with - patch doesn't apply successfully at *.md file. It does not reproduce locally. Anybody knows what it is? https://codereview.chromium.org/2944523002/diff/100001/base/containers/README.md File base/containers/README.md (right): https://codereview.chromium.org/2944523002/diff/100001/base/containers/README... base/containers/README.md:127: flat\_set/flat\_map support C++14 interface and a notion of transparent On 2017/06/26 at 19:33:12, danakj wrote: > Can you add a code example here of using flat_map or flat_set with unique_ptr and with std::string? And once this lands, also send an email to chromium-dev including those examples in it? I did. Seems weird not to actually provide a set for smart pointers. https://codereview.chromium.org/2944523002/diff/100001/base/containers/README... base/containers/README.md:127: flat\_set/flat\_map support C++14 interface and a notion of transparent On 2017/06/26 at 19:33:12, danakj wrote: > Can you add a code example here of using flat_map or flat_set with unique_ptr and with std::string? And once this lands, also send an email to chromium-dev including those examples in it? I did. Looks weird without actually providing a solution for ptr_set. https://codereview.chromium.org/2944523002/diff/100001/base/containers/flat_m... File base/containers/flat_map_unittest.cc (right): https://codereview.chromium.org/2944523002/diff/100001/base/containers/flat_m... base/containers/flat_map_unittest.cc:173: TEST(FlatMap, UniquePtrKeys) { On 2017/06/26 at 19:33:12, danakj wrote: > nit: there's no UniquePtrs in this test, CompareWithUnderlyingType or something? Done https://codereview.chromium.org/2944523002/diff/100001/base/containers/flat_s... File base/containers/flat_set_unittest.cc (right): https://codereview.chromium.org/2944523002/diff/100001/base/containers/flat_s... base/containers/flat_set_unittest.cc:93: TEST(FlatSet, UniquePtrs) { On 2017/06/26 at 19:33:12, danakj wrote: > same nit Done https://codereview.chromium.org/2944523002/diff/100001/base/containers/flat_t... File base/containers/flat_tree.h (right): https://codereview.chromium.org/2944523002/diff/100001/base/containers/flat_t... base/containers/flat_tree.h:913: // Only compile when conversion is implicit. On 2017/06/26 at 19:33:12, danakj wrote: > What does the compiler error look like when this fails? If you type: ``` base::flat_set<std::unique_ptr<int>, std::less<std::unique_ptr<int>>> test; test.lower_bound(&x); ``` You get a compilation error: ``` ../../base/containers/flat_tree.h:916:24: error: reference to type 'const KeyTypeOrK<int *>' (aka 'const std::__1::unique_ptr<int, std::__1::default_delete<int> >') could not bind to an lvalue of type 'int *const' const KeyTypeOrK<K>& key_ref = key; ``` https://codereview.chromium.org/2944523002/diff/100001/base/functional.h File base/functional.h (right): https://codereview.chromium.org/2944523002/diff/100001/base/functional.h#newc... base/functional.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/06/26 at 19:33:12, danakj wrote: > Can this go in template_util.h? We haven't reproduced the STL header structure in base and I'm not sure that we want to start now. Done https://codereview.chromium.org/2944523002/diff/100001/base/functional.h#newc... base/functional.h:14: // Needed for improved flat containers API. On 2017/06/26 at 19:33:12, danakj wrote: > remove this line Done. https://codereview.chromium.org/2944523002/diff/100001/base/functional.h#newc... base/functional.h:23: using is_transparent = int; On 2017/06/26 at 19:33:12, danakj wrote: > Refer to transparent docs here also please Done https://codereview.chromium.org/2944523002/diff/100001/base/functional.h#newc... base/functional.h:26: } // base On 2017/06/26 at 19:33:12, danakj wrote: > namespace base Not applicable. https://codereview.chromium.org/2944523002/diff/100001/base/functional_unitte... File base/functional_unittest.cc (right): https://codereview.chromium.org/2944523002/diff/100001/base/functional_unitte... base/functional_unittest.cc:7: #include "base/containers/container_test_utils.h" On 2017/06/26 at 19:33:12, danakj wrote: > this file should be in base/test/, especially if it's going to be exporting types. > > really as base/test/move_only_int.h Done. https://codereview.chromium.org/2944523002/diff/100001/base/functional_unitte... base/functional_unittest.cc:12: On 2017/06/26 at 19:33:12, danakj wrote: > nit: remove whitespace Not applicable. https://codereview.chromium.org/2944523002/diff/100001/base/functional_unitte... base/functional_unittest.cc:25: On 2017/06/26 at 19:33:12, danakj wrote: > nit: remove whitespace Not applicable. https://codereview.chromium.org/2944523002/diff/120001/base/containers/README.md File base/containers/README.md (right): https://codereview.chromium.org/2944523002/diff/120001/base/containers/README... base/containers/README.md:184: str_to_int["c"] = 3; standard doesn't provide a templated overload for operator[]. I don't know how to do it well - therefor I don't think we should. (type K has to be comparable with key_type, not necessarily convertible)
https://codereview.chromium.org/2944523002/diff/120001/base/containers/README.md File base/containers/README.md (right): https://codereview.chromium.org/2944523002/diff/120001/base/containers/README... base/containers/README.md:49: where you make many objects so that the code/heap tradeoff is good. I wonder if this is causing the patch failure, it seems like a no-op change? Is that a part of your local diff? https://codereview.chromium.org/2944523002/diff/120001/base/containers/README... base/containers/README.md:184: str_to_int["c"] = 3; On 2017/06/26 22:40:43, dyaroshev wrote: > standard doesn't provide a templated overload for operator[]. I don't know how > to do it well - therefor I don't think we should. > > (type K has to be comparable with key_type, not necessarily convertible) I agree. I think that if you have a transparent comparator it doesn't have to imply that key_type is constructible from K. Can you expand your NOTE about to provide this reason? Something like "NOTE: This does construct a temporary string. This happens since if the item is not in the container, then it needs to be constructed, which is something that transparent comparators don't have to guarantee." I kind of tried to elaborate more, but it gets very detailed quickly, so I think that comment is fine... https://codereview.chromium.org/2944523002/diff/120001/base/containers/README... base/containers/README.md:186: // Do not construct temporary strings. nit: s/Do/Does/
The CQ bit was checked by dyaroshev@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
https://codereview.chromium.org/2944523002/diff/120001/base/containers/README.md File base/containers/README.md (right): https://codereview.chromium.org/2944523002/diff/120001/base/containers/README... base/containers/README.md:49: where you make many objects so that the code/heap tradeoff is good. On 2017/06/26 23:06:34, vmpstr wrote: > I wonder if this is causing the patch failure, it seems like a no-op change? Is > that a part of your local diff? Investigating locally this seems to be replacing a non-breaking space (U+00A0) with a regular space (U+0020). Further evidence for a non-breaking space here can be found by investigating the corresponding sentence in https://chromium.googlesource.com/chromium/src/+/master/base/containers/READM... with dev tools. It's odd that patch seems to be unable to find this space and thus the bots break. Probably a bug should be filed to investigate this further, but maybe simply revert to a non-breaking space in the meantime?
https://codereview.chromium.org/2944523002/diff/100001/base/containers/flat_t... File base/containers/flat_tree.h (right): https://codereview.chromium.org/2944523002/diff/100001/base/containers/flat_t... base/containers/flat_tree.h:913: // Only compile when conversion is implicit. On 2017/06/26 22:40:42, dyaroshev wrote: > On 2017/06/26 at 19:33:12, danakj wrote: > > What does the compiler error look like when this fails? > > If you type: > ``` > base::flat_set<std::unique_ptr<int>, std::less<std::unique_ptr<int>>> test; > test.lower_bound(&x); > ``` > > You get a compilation error: > ``` > ../../base/containers/flat_tree.h:916:24: error: reference to type 'const > KeyTypeOrK<int *>' (aka 'const std::__1::unique_ptr<int, > std::__1::default_delete<int> >') could not bind to an lvalue of type 'int > *const' > const KeyTypeOrK<K>& key_ref = key; > ``` We talked a bit about this. The error is somewhat hard to understand without knowing the internals of how flat_tree is implemented. We came up with a couple options to try to see if the error looks better. Do you mind doing one of these? (or anything else you come up with that might be easier to understand from the user's point of view) 1. Try to enable_if this function based on whether the comparator is transparent and if it's not transparent, then only have a key_type& override. I think that will probably give a better error, but more code to type. 2. Static assert that we can create this type. In this one, the phrasing of the error is up to us, so maybe something like "Requested type cannot be bound to the container's key_type".
On 2017/06/27 at 16:09:58, vmpstr wrote: > https://codereview.chromium.org/2944523002/diff/100001/base/containers/flat_t... > File base/containers/flat_tree.h (right): > > https://codereview.chromium.org/2944523002/diff/100001/base/containers/flat_t... > base/containers/flat_tree.h:913: // Only compile when conversion is implicit. > On 2017/06/26 22:40:42, dyaroshev wrote: > > On 2017/06/26 at 19:33:12, danakj wrote: > > > What does the compiler error look like when this fails? > > > > If you type: > > ``` > > base::flat_set<std::unique_ptr<int>, std::less<std::unique_ptr<int>>> test; > > test.lower_bound(&x); > > ``` > > > > You get a compilation error: > > ``` > > ../../base/containers/flat_tree.h:916:24: error: reference to type 'const > > KeyTypeOrK<int *>' (aka 'const std::__1::unique_ptr<int, > > std::__1::default_delete<int> >') could not bind to an lvalue of type 'int > > *const' > > const KeyTypeOrK<K>& key_ref = key; > > ``` > > We talked a bit about this. The error is somewhat hard to understand without knowing the internals of how flat_tree is implemented. We came up with a couple options to try to see if the error looks better. Do you mind doing one of these? (or anything else you come up with that might be easier to understand from the user's point of view) > > 1. Try to enable_if this function based on whether the comparator is transparent and if it's not transparent, then only have a key_type& override. I think that will probably give a better error, but more code to type. > > 2. Static assert that we can create this type. In this one, the phrasing of the error is up to us, so maybe something like "Requested type cannot be bound to the container's key_type". Done
https://codereview.chromium.org/2944523002/diff/120001/base/containers/README.md File base/containers/README.md (right): https://codereview.chromium.org/2944523002/diff/120001/base/containers/README... base/containers/README.md:184: str_to_int["c"] = 3; On 2017/06/26 at 23:06:34, vmpstr wrote: > On 2017/06/26 22:40:43, dyaroshev wrote: > > standard doesn't provide a templated overload for operator[]. I don't know how > > to do it well - therefor I don't think we should. > > > > (type K has to be comparable with key_type, not necessarily convertible) > > I agree. I think that if you have a transparent comparator it doesn't have to imply that key_type is constructible from K. Can you expand your NOTE about to provide this reason? Something like > > "NOTE: This does construct a temporary string. This happens since if the item is not in the container, then it needs to be constructed, which is something that transparent comparators don't have to guarantee." > > I kind of tried to elaborate more, but it gets very detailed quickly, so I think that comment is fine... Done https://codereview.chromium.org/2944523002/diff/120001/base/containers/README... base/containers/README.md:186: // Do not construct temporary strings. On 2017/06/26 at 23:06:34, vmpstr wrote: > nit: s/Do/Does/ Done
The CQ bit was checked by dyaroshev@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/06/26 19:16:54, dyaroshev wrote:
> On 2017/06/26 at 19:13:26, danakj wrote:
> > On Mon, Jun 26, 2017 at 3:10 PM, <mailto:dyaroshev@yandex-team.ru> wrote:
> >
> > > On 2017/06/26 at 19:05:54, vmpstr wrote:
> > > > What happens if you have something like flat_set<int> and then do a
> > > find(1.5f); on it? Does that compile and work?
> > > >
> > > > I believe some components have narrowing conversion warnings enabled,
> > > but if
> > > it's transparent would that just compare an int to a float?
> > >
> > > I checked in base_unittests with clang, you are correct - compiles and
> > > runs. I
> > > recently got a waring from MSVS, maybe it will catch such a problem?
> > >
> > > I don't know, do you have something in mind for this issue?
> > >
> >
> > Is there any existing code that depends on this behaviour? Can you make it
> > not compile to find it with bots? If not then we can change it, if there is
> > we need to consider them/fix them.
> >
>
> I started bot's run, but I don't exactly see in what's way. Do you mean than
> before this patch looking up 1.5f in a set {1, 2} would return 1, and now two?
> Interesting, crasy a bit though.
Did this uncover anything?
https://codereview.chromium.org/2944523002/diff/160001/base/containers/README.md
File base/containers/README.md (right):
https://codereview.chromium.org/2944523002/diff/160001/base/containers/README...
base/containers/README.md:168: Example flat_map<\std::string, int>:
accidental \ ?
https://codereview.chromium.org/2944523002/diff/160001/base/containers/flat_m...
File base/containers/flat_map_unittest.cc (right):
https://codereview.chromium.org/2944523002/diff/160001/base/containers/flat_m...
base/containers/flat_map_unittest.cc:207: // Do not construct temporary strings.
If we're not testing somehow the temp strings are not being constructed, then
these don't belong as unit tests. The examples don't need to be unit tests, we
can leave them in the README.md alone.
https://codereview.chromium.org/2944523002/diff/160001/base/containers/flat_s...
File base/containers/flat_set_unittest.cc (right):
https://codereview.chromium.org/2944523002/diff/160001/base/containers/flat_s...
base/containers/flat_set_unittest.cc:150: TEST(FlatSet, Examples) {
same
https://codereview.chromium.org/2944523002/diff/160001/base/template_util_uni...
File base/template_util_unittest.cc (right):
https://codereview.chromium.org/2944523002/diff/160001/base/template_util_uni...
base/template_util_unittest.cc:76: TEST(Functors, Less) {
nit: TemplateUtil.Less would be easier to find
The CQ bit was checked by dyaroshev@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #10 (id:180001) has been deleted
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2944523002/diff/160001/base/containers/README.md File base/containers/README.md (right): https://codereview.chromium.org/2944523002/diff/160001/base/containers/README... base/containers/README.md:49: where you make many objects so that the code/heap tradeoff is good. This seems to have fixed the patch issue. Currently, the bots are still complaining because you removed container_test_utils.h but it is still mentioned in base/BUILD.gn
The CQ bit was checked by dyaroshev@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On danakj@: - Did this uncover anything? Haven't defeated other issues yet. On jdoerrie: - This seems to have fixed the patch issue. Currently, the bots are still complaining because you removed container_test_utils.h but it is still mentioned in base/BUILD.gn Yes, I had to checkout file from master. Fixed the new thing too, thanks. https://codereview.chromium.org/2944523002/diff/160001/base/containers/README.md File base/containers/README.md (right): https://codereview.chromium.org/2944523002/diff/160001/base/containers/README... base/containers/README.md:168: Example flat_map<\std::string, int>: On 2017/06/27 at 23:48:25, danakj wrote: > accidental \ ? std::string highlights for some reason. Here it highlights anyways. Done. https://codereview.chromium.org/2944523002/diff/160001/base/containers/flat_m... File base/containers/flat_map_unittest.cc (right): https://codereview.chromium.org/2944523002/diff/160001/base/containers/flat_m... base/containers/flat_map_unittest.cc:207: // Do not construct temporary strings. On 2017/06/27 at 23:48:25, danakj wrote: > If we're not testing somehow the temp strings are not being constructed, then these don't belong as unit tests. The examples don't need to be unit tests, we can leave them in the README.md alone. Done https://codereview.chromium.org/2944523002/diff/160001/base/containers/flat_s... File base/containers/flat_set_unittest.cc (right): https://codereview.chromium.org/2944523002/diff/160001/base/containers/flat_s... base/containers/flat_set_unittest.cc:150: TEST(FlatSet, Examples) { On 2017/06/27 at 23:48:25, danakj wrote: > same Done https://codereview.chromium.org/2944523002/diff/160001/base/template_util_uni... File base/template_util_unittest.cc (right): https://codereview.chromium.org/2944523002/diff/160001/base/template_util_uni... base/template_util_unittest.cc:76: TEST(Functors, Less) { On 2017/06/27 at 23:48:25, danakj wrote: > nit: TemplateUtil.Less would be easier to find Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Description was changed from ========== Improving flat containers interface. This patch adds a few features to base::flat_set/base::flat_map. 1) C++14 overloads of search operations. Now count, find, equal_range, lower_bound, upper_bound accept any type comparable with key_type if the comparison is transparent. Comparison is considered to be transparent if it has `is_transparent` member typedef (http://en.cppreference.com/w/cpp/utility/functional/less_void) We removed non-templated versions, because they were mainly for backward compatibility in extremely rare cases. 2) Templated version of erase. This version was in the original proposal but later was removed as a result of a bug report, because it changed overload resolution for `erase(iterator)`. We provide an explicit `erase(iterator)` overload that addresses this issue. BUG=682254 ========== to ========== Improving flat containers interface. This patch adds a few features to base::flat_set/base::flat_map. 1) C++14 overloads of search operations. Now count, find, equal_range, lower_bound, upper_bound accept any type comparable with key_type if the comparison is transparent. Comparison is considered to be transparent if it has `is_transparent` member typedef (http://en.cppreference.com/w/cpp/utility/functional/less_void) We removed non-templated versions, because they were mainly for backward compatibility in extremely rare cases. 2) Templated version of erase. This version was in the original proposal but later was removed as a result of a bug report, because it changed overload resolution for `erase(iterator)`. We provide an explicit `erase(iterator)` overload that addresses this issue. BUG=682254 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
On 2017/06/27 at 23:48:25, danakj wrote: > Did this uncover anything? Apparently using template arguments requires stricter type checking: it's ok to convert int to int64_t, but it's not ok to compare them. So far fixed chrome target.
The CQ bit was checked by dyaroshev@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
On 2017/06/28 20:58:52, dyaroshev wrote: > On 2017/06/27 at 23:48:25, danakj wrote: > > Did this uncover anything? > > Apparently using template arguments requires stricter type checking: it's ok to > convert int to int64_t, but it's not ok to compare them. Weird.. what error do you get? > > So far fixed chrome target. It's fine to combine this to run through trybots, but for the sake of landing, can you separate the non-base changes to separate CL(s)? That way if this needs to be reverted, then at least the cleanup patches in other components don't have to be (and it makes reviews easier).
On 2017/06/28 at 21:38:42, vmpstr wrote: > On 2017/06/28 20:58:52, dyaroshev wrote: > > On 2017/06/27 at 23:48:25, danakj wrote: > > > Did this uncover anything? > > > > Apparently using template arguments requires stricter type checking: it's ok to > > convert int to int64_t, but it's not ok to compare them. > > Weird.. what error do you get? Comparing int to long long, smth like that I think. Yeah, seems wrong. There were a few of those. > > > > > So far fixed chrome target. > > It's fine to combine this to run through trybots, but for the sake of landing, can you separate the non-base changes to separate CL(s)? That way if this needs to be reverted, then at least the cleanup patches in other components don't have to be (and it makes reviews easier). It's somewhat hard - switching between branches takes forever (I have to recompile the whole thing). Do we really need this? There are not that many changes.
On 2017/06/28 23:26:16, dyaroshev wrote: > On 2017/06/28 at 21:38:42, vmpstr wrote: > > On 2017/06/28 20:58:52, dyaroshev wrote: > > > On 2017/06/27 at 23:48:25, danakj wrote: > > > > Did this uncover anything? > > > > > > Apparently using template arguments requires stricter type checking: it's ok > to > > > convert int to int64_t, but it's not ok to compare them. > > > > Weird.. what error do you get? > > Comparing int to long long, smth like that I think. Yeah, seems wrong. There > were a few of those. > > > > > > > > > So far fixed chrome target. > > > > It's fine to combine this to run through trybots, but for the sake of landing, > can you separate the non-base changes to separate CL(s)? That way if this needs > to be reverted, then at least the cleanup patches in other components don't have > to be (and it makes reviews easier). > > It's somewhat hard - switching between branches takes forever (I have to > recompile the whole thing). Do we really need this? There are not that many > changes. If there aren't many changes, then I think it's fine.
On 2017/06/28 20:58:52, dyaroshev wrote: > On 2017/06/27 at 23:48:25, danakj wrote: > > Did this uncover anything? > > Apparently using template arguments requires stricter type checking: it's ok to > convert int to int64_t, but it's not ok to compare them. > > So far fixed chrome target. I was thinking a bit more about this. Does this mean that if I have something like std::flat_set<int64_t> set; and I do set.find(0); I will get an error? Or worse, if the flat_set contains size_ts, then I _might_ get an error depending on the sizeof(size_t)? I think that's a bit concerning from the users' perspectives. Is the error coming from base::less?
On 2017/06/29 20:04:23, vmpstr wrote: > On 2017/06/28 20:58:52, dyaroshev wrote: > > On 2017/06/27 at 23:48:25, danakj wrote: > > > Did this uncover anything? > > > > Apparently using template arguments requires stricter type checking: it's ok > to > > convert int to int64_t, but it's not ok to compare them. > > > > So far fixed chrome target. > > I was thinking a bit more about this. Does this mean that if I have something > like > > std::flat_set<int64_t> set; I, of course, meant base:: not std:: > > and I do > > set.find(0); > > I will get an error? > > Or worse, if the flat_set contains size_ts, then I _might_ get an error > depending on the sizeof(size_t)? > > I think that's a bit concerning from the users' perspectives. Is the error > coming from base::less?
On 2017/06/29 at 20:05:04, vmpstr wrote: > On 2017/06/29 20:04:23, vmpstr wrote: > > On 2017/06/28 20:58:52, dyaroshev wrote: > > > On 2017/06/27 at 23:48:25, danakj wrote: > > > > Did this uncover anything? > > > > > > Apparently using template arguments requires stricter type checking: it's ok > > to > > > convert int to int64_t, but it's not ok to compare them. > > > > > > So far fixed chrome target. > > > > I was thinking a bit more about this. Does this mean that if I have something > > like > > > > std::flat_set<int64_t> set; > > I, of course, meant base:: not std:: > > > > > and I do > > > > set.find(0); > > > > I will get an error? > > > > Or worse, if the flat_set contains size_ts, then I _might_ get an error > > depending on the sizeof(size_t)? > > > > I think that's a bit concerning from the users' perspectives. Is the error > > coming from base::less? Yes to find(0). With size_t you'll get signed/unsigned comparison error. I'll look into it and see if I can do something from within to accept more cases.
On 2017/06/29 21:19:28, dyaroshev wrote:
> On 2017/06/29 at 20:05:04, vmpstr wrote:
> > On 2017/06/29 20:04:23, vmpstr wrote:
> > > On 2017/06/28 20:58:52, dyaroshev wrote:
> > > > On 2017/06/27 at 23:48:25, danakj wrote:
> > > > > Did this uncover anything?
> > > >
> > > > Apparently using template arguments requires stricter type checking:
it's
> ok
> > > to
> > > > convert int to int64_t, but it's not ok to compare them.
> > > >
> > > > So far fixed chrome target.
> > >
> > > I was thinking a bit more about this. Does this mean that if I have
> something
> > > like
> > >
> > > std::flat_set<int64_t> set;
> >
> > I, of course, meant base:: not std::
> >
> > >
> > > and I do
> > >
> > > set.find(0);
> > >
> > > I will get an error?
> > >
> > > Or worse, if the flat_set contains size_ts, then I _might_ get an error
> > > depending on the sizeof(size_t)?
> > >
> > > I think that's a bit concerning from the users' perspectives. Is the error
> > > coming from base::less?
>
> Yes to find(0). With size_t you'll get signed/unsigned comparison error. I'll
> look into it and see if I can do something from within to accept more cases.
Are you sure about this? I just tried to compile
std::flat_set<int64_t> set;
set.find(0);
with this patch and it worked fine.
The case in input_proxy_handler.cc is different, because the int is part of an
std::pair.
Trying to compile that with 0 instead of INT64_C(0) I get the following error on
a 64-bit Linux machine:
substitution failure [with T = const std::pair<ui::LatencyComponentType, long>
&, U = const std::pair<ui::LatencyComponentType, int> &]: invalid operands to
binary expression ('const std::pair<ui::LatencyComponentType, long>' and 'const
std::pair<ui::LatencyComponentType, int>')
On 2017/06/30 at 08:35:51, jdoerrie wrote:
> On 2017/06/29 21:19:28, dyaroshev wrote:
> > On 2017/06/29 at 20:05:04, vmpstr wrote:
> > > On 2017/06/29 20:04:23, vmpstr wrote:
> > > > On 2017/06/28 20:58:52, dyaroshev wrote:
> > > > > On 2017/06/27 at 23:48:25, danakj wrote:
> > > > > > Did this uncover anything?
> > > > >
> > > > > Apparently using template arguments requires stricter type checking:
it's
> > ok
> > > > to
> > > > > convert int to int64_t, but it's not ok to compare them.
> > > > >
> > > > > So far fixed chrome target.
> > > >
> > > > I was thinking a bit more about this. Does this mean that if I have
> > something
> > > > like
> > > >
> > > > std::flat_set<int64_t> set;
> > >
> > > I, of course, meant base:: not std::
> > >
> > > >
> > > > and I do
> > > >
> > > > set.find(0);
> > > >
> > > > I will get an error?
> > > >
> > > > Or worse, if the flat_set contains size_ts, then I _might_ get an error
> > > > depending on the sizeof(size_t)?
> > > >
> > > > I think that's a bit concerning from the users' perspectives. Is the
error
> > > > coming from base::less?
> >
> > Yes to find(0). With size_t you'll get signed/unsigned comparison error.
I'll
> > look into it and see if I can do something from within to accept more cases.
>
> Are you sure about this? I just tried to compile
> std::flat_set<int64_t> set;
> set.find(0);
> with this patch and it worked fine.
>
> The case in input_proxy_handler.cc is different, because the int is part of an
std::pair.
> Trying to compile that with 0 instead of INT64_C(0) I get the following error
on a 64-bit Linux machine:
>
> substitution failure [with T = const std::pair<ui::LatencyComponentType, long>
&, U = const std::pair<ui::LatencyComponentType, int> &]: invalid operands to
binary expression ('const std::pair<ui::LatencyComponentType, long>' and 'const
std::pair<ui::LatencyComponentType, int>')
Oh, yeah, you are right.
I suspect that the error message was slightly longer than that)))
The CQ bit was checked by dyaroshev@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
The CQ bit was checked by dyaroshev@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dyaroshev@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by dyaroshev@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
I hope the final patch should pass builds. I dealt with compilation issues. The only questionable new change here is target_auto_attacher.cc, I think it's small enough though and it's better this way. Let's review and land?
https://codereview.chromium.org/2944523002/diff/300001/content/browser/devtoo... File content/browser/devtools/protocol/target_auto_attacher.cc (right): https://codereview.chromium.org/2944523002/diff/300001/content/browser/devtoo... content/browser/devtools/protocol/target_auto_attacher.cc:172: for (const auto& h : removed_hosts) And I missed GetType check. Ok than, I'll revert this and just fix the compilation.
https://codereview.chromium.org/2944523002/diff/300001/content/browser/devtoo... File content/browser/devtools/protocol/target_auto_attacher.cc (right): https://codereview.chromium.org/2944523002/diff/300001/content/browser/devtoo... content/browser/devtools/protocol/target_auto_attacher.cc:137: auto_attached_hosts_.erase(make_scoped_refptr(host)); Not for this CL but looks like we should make a comparator for scoped_refptr keys, and use it for all applicable flat set/maps, so we don't have to do this. https://codereview.chromium.org/2944523002/diff/300001/content/browser/devtoo... content/browser/devtools/protocol/target_auto_attacher.cc:172: for (const auto& h : removed_hosts) On 2017/07/19 23:38:41, dyaroshev wrote: > And I missed GetType check. Ok than, I'll revert this and just fix the > compilation. I'll wait for your update then.
https://codereview.chromium.org/2944523002/diff/300001/content/browser/devtoo... File content/browser/devtools/protocol/target_auto_attacher.cc (right): https://codereview.chromium.org/2944523002/diff/300001/content/browser/devtoo... content/browser/devtools/protocol/target_auto_attacher.cc:137: auto_attached_hosts_.erase(make_scoped_refptr(host)); On 2017/07/20 at 15:52:52, danakj wrote: > Not for this CL but looks like we should make a comparator for scoped_refptr keys, and use it for all applicable flat set/maps, so we don't have to do this. I agree. We discussed that earlier in this CL, I even had a general purpose solution in earlier version of this CL.
On 2017/07/20 at 15:52:52, danakj wrote: > https://codereview.chromium.org/2944523002/diff/300001/content/browser/devtoo... > File content/browser/devtools/protocol/target_auto_attacher.cc (right): > > https://codereview.chromium.org/2944523002/diff/300001/content/browser/devtoo... > content/browser/devtools/protocol/target_auto_attacher.cc:137: auto_attached_hosts_.erase(make_scoped_refptr(host)); > Not for this CL but looks like we should make a comparator for scoped_refptr keys, and use it for all applicable flat set/maps, so we don't have to do this. > > https://codereview.chromium.org/2944523002/diff/300001/content/browser/devtoo... > content/browser/devtools/protocol/target_auto_attacher.cc:172: for (const auto& h : removed_hosts) > On 2017/07/19 23:38:41, dyaroshev wrote: > > And I missed GetType check. Ok than, I'll revert this and just fix the > > compilation. > > I'll wait for your update then. Done.
The CQ bit was checked by dyaroshev@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2944523002/diff/320001/cc/output/direct_rende... File cc/output/direct_renderer.cc (right): https://codereview.chromium.org/2944523002/diff/320001/cc/output/direct_rende... cc/output/direct_renderer.cc:196: [&](const std::pair<RenderPassId, can you put the lambda in an auto var first, rather than nesting it all inside EraseIf? and maybe give it a name and/or comment that's clear this function is doing more than an EraseIf comparison, it's also freeing textures of the wrong size, cuz that seems surprising from an EraseIf() call. Alternatively, if that seems hard to express, since these vectors are of sizes around 1 to 3 elements.. you could split this into EraseIf() and a separate loop for size checks. https://codereview.chromium.org/2944523002/diff/320001/content/browser/devtoo... File content/browser/devtools/protocol/target_auto_attacher.cc (right): https://codereview.chromium.org/2944523002/diff/320001/content/browser/devtoo... content/browser/devtools/protocol/target_auto_attacher.cc:178: auto_attached_hosts_.insert(host.get()); unclear to me why you get the raw pointer to insert here, when the set is refptrs?
https://codereview.chromium.org/2944523002/diff/320001/cc/output/direct_rende... File cc/output/direct_renderer.cc (right): https://codereview.chromium.org/2944523002/diff/320001/cc/output/direct_rende... cc/output/direct_renderer.cc:196: [&](const std::pair<RenderPassId, On 2017/07/20 at 16:12:20, danakj wrote: > can you put the lambda in an auto var first, rather than nesting it all inside EraseIf? > > and maybe give it a name and/or comment that's clear this function is doing more than an EraseIf comparison, it's also freeing textures of the wrong size, cuz that seems surprising from an EraseIf() call. > > Alternatively, if that seems hard to express, since these vectors are of sizes around 1 to 3 elements.. you could split this into EraseIf() and a separate loop for size checks. Just reverted and fixed compilation. https://codereview.chromium.org/2944523002/diff/320001/content/browser/devtoo... File content/browser/devtools/protocol/target_auto_attacher.cc (right): https://codereview.chromium.org/2944523002/diff/320001/content/browser/devtoo... content/browser/devtools/protocol/target_auto_attacher.cc:178: auto_attached_hosts_.insert(host.get()); On 2017/07/20 at 16:12:20, danakj wrote: > unclear to me why you get the raw pointer to insert here, when the set is refptrs? Sorry.
LGTM. Thanks for making sure nothing breaks from this!
The CQ bit was checked by dyaroshev@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from jdoerrie@chromium.org, vmpstr@chromium.org Link to the patchset: https://codereview.chromium.org/2944523002/#ps340001 (title: "Review, minimizing diff 2.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
dyaroshev@yandex-team.ru changed reviewers: + sky@chromium.org, thakis@chromium.org
sky - please review, target_auto_attacher thakis - please review input_handler_proxy
File file lgtm, but: https://codereview.chromium.org/2944523002/diff/340001/base/containers/README.md File base/containers/README.md (right): https://codereview.chromium.org/2944523002/diff/340001/base/containers/README... base/containers/README.md:128: flat\_set/flat\_map support C++14 interface and a notion of transparent We're not on c++14 yet, and we don't know how and when we're going to switch to it. Why do we mention c++14 in this doc (and why are we adding c++-14-related stuff to the code base?)
*My file lgtm, but:
https://codereview.chromium.org/2944523002/diff/340001/base/containers/README.md File base/containers/README.md (right): https://codereview.chromium.org/2944523002/diff/340001/base/containers/README... base/containers/README.md:128: flat\_set/flat\_map support C++14 interface and a notion of transparent On 2017/07/20 18:26:53, Nico wrote: > We're not on c++14 yet, and we don't know how and when we're going to switch to > it. Why do we mention c++14 in this doc (and why are we adding c++-14-related > stuff to the code base?) Do you think mentioning c++14 is bad? It gives a really helpful tool to help understand how this works to compare to other things IMO. This is being added because it is useful, not because it is in c++14, but being done in a way that will be familiar with other c++14 containers. This allows you to key your set with a unique_ptr, instead of having to do flat_map<T*, unqiue_ptr<T>> for example, or avoid constructing temporary std::strings on every lookup.
On Thu, Jul 20, 2017 at 2:51 PM, <danakj@chromium.org> wrote: > > https://codereview.chromium.org/2944523002/diff/340001/ > base/containers/README.md > File base/containers/README.md (right): > > https://codereview.chromium.org/2944523002/diff/340001/ > base/containers/README.md#newcode128 > base/containers/README.md:128: flat\_set/flat\_map support C++14 > interface and a notion of transparent > On 2017/07/20 18:26:53, Nico wrote: > > We're not on c++14 yet, and we don't know how and when we're going to > switch to > > it. Why do we mention c++14 in this doc (and why are we adding > c++-14-related > > stuff to the code base?) > > Do you think mentioning c++14 is bad? > I think it's a bit confusing, yes. > It gives a really helpful tool to > help understand how this works to compare to other things IMO. > > This is being added because it is useful, not because it is in c++14, > but being done in a way that will be familiar with other c++14 > containers. This allows you to key your set with a unique_ptr, instead > of having to do flat_map<T*, unqiue_ptr<T>> for example, or avoid > constructing temporary std::strings on every lookup. > > https://codereview.chromium.org/2944523002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Thu, Jul 20, 2017 at 3:01 PM, Nico Weber <thakis@chromium.org> wrote: > On Thu, Jul 20, 2017 at 2:51 PM, <danakj@chromium.org> wrote: > >> >> https://codereview.chromium.org/2944523002/diff/340001/base/ >> containers/README.md >> File base/containers/README.md (right): >> >> https://codereview.chromium.org/2944523002/diff/340001/base/ >> containers/README.md#newcode128 >> base/containers/README.md:128: flat\_set/flat\_map support C++14 >> interface and a notion of transparent >> On 2017/07/20 18:26:53, Nico wrote: >> > We're not on c++14 yet, and we don't know how and when we're going to >> switch to >> > it. Why do we mention c++14 in this doc (and why are we adding >> c++-14-related >> > stuff to the code base?) >> >> Do you think mentioning c++14 is bad? >> > > I think it's a bit confusing, yes. > Is it better if it's phrased differently like "supports this and that, similar to c++14 containers"? Or you're against any mention of it? Notably I guess, this is quite common already it seems.. - optional.h mentions its an implementation based on the class in c++17 - tuple.h mentions it's a clone of C++14 functionality - bind_internal.h mentions it has a copy of c++17 void_t - ptr_util.h points out MakeUnique is from c++14 - aligned_memory.h says that it is a substitute for functionality in c++17. I can't measure the number of cases that could have but avoided mentioning future c++ specs though. > > >> It gives a really helpful tool to >> help understand how this works to compare to other things IMO. >> >> This is being added because it is useful, not because it is in c++14, >> but being done in a way that will be familiar with other c++14 >> containers. This allows you to key your set with a unique_ptr, instead >> of having to do flat_map<T*, unqiue_ptr<T>> for example, or avoid >> constructing temporary std::strings on every lookup. >> >> https://codereview.chromium.org/2944523002/ >> > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Thu, Jul 20, 2017 at 3:06 PM, <danakj@chromium.org> wrote: > On Thu, Jul 20, 2017 at 3:01 PM, Nico Weber <thakis@chromium.org> wrote: > >> On Thu, Jul 20, 2017 at 2:51 PM, <danakj@chromium.org> wrote: >> >>> >>> https://codereview.chromium.org/2944523002/diff/340001/base/ >>> containers/README.md >>> File base/containers/README.md (right): >>> >>> https://codereview.chromium.org/2944523002/diff/340001/base/ >>> containers/README.md#newcode128 >>> base/containers/README.md:128: flat\_set/flat\_map support C++14 >>> interface and a notion of transparent >>> On 2017/07/20 18:26:53, Nico wrote: >>> > We're not on c++14 yet, and we don't know how and when we're going to >>> switch to >>> > it. Why do we mention c++14 in this doc (and why are we adding >>> c++-14-related >>> > stuff to the code base?) >>> >>> Do you think mentioning c++14 is bad? >>> >> >> I think it's a bit confusing, yes. >> > > Is it better if it's phrased differently like "supports this and that, > similar to c++14 containers"? Or you're against any mention of it? > Your rephrased suggestion is much better, imho. I'd be happy with that. > > Notably I guess, this is quite common already it seems.. > - optional.h mentions its an implementation based on the class in c++17 > - tuple.h mentions it's a clone of C++14 functionality > - bind_internal.h mentions it has a copy of c++17 void_t > - ptr_util.h points out MakeUnique is from c++14 > - aligned_memory.h says that it is a substitute for functionality in c++17. > Right, they all say "clone" or "replacement', but this doc here doesn't read like that to me. (With your suggestion, it does.) > > I can't measure the number of cases that could have but avoided mentioning > future c++ specs though. > > >> >> >>> It gives a really helpful tool to >>> help understand how this works to compare to other things IMO. >>> >>> This is being added because it is useful, not because it is in c++14, >>> but being done in a way that will be familiar with other c++14 >>> containers. This allows you to key your set with a unique_ptr, instead >>> of having to do flat_map<T*, unqiue_ptr<T>> for example, or avoid >>> constructing temporary std::strings on every lookup. >>> >>> https://codereview.chromium.org/2944523002/ >>> >> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Thu, Jul 20, 2017 at 3:10 PM, Nico Weber <thakis@chromium.org> wrote: > On Thu, Jul 20, 2017 at 3:06 PM, <danakj@chromium.org> wrote: > >> On Thu, Jul 20, 2017 at 3:01 PM, Nico Weber <thakis@chromium.org> wrote: >> >>> On Thu, Jul 20, 2017 at 2:51 PM, <danakj@chromium.org> wrote: >>> >>>> >>>> https://codereview.chromium.org/2944523002/diff/340001/base/ >>>> containers/README.md >>>> File base/containers/README.md (right): >>>> >>>> https://codereview.chromium.org/2944523002/diff/340001/base/ >>>> containers/README.md#newcode128 >>>> base/containers/README.md:128: flat\_set/flat\_map support C++14 >>>> interface and a notion of transparent >>>> On 2017/07/20 18:26:53, Nico wrote: >>>> > We're not on c++14 yet, and we don't know how and when we're going to >>>> switch to >>>> > it. Why do we mention c++14 in this doc (and why are we adding >>>> c++-14-related >>>> > stuff to the code base?) >>>> >>>> Do you think mentioning c++14 is bad? >>>> >>> >>> I think it's a bit confusing, yes. >>> >> >> Is it better if it's phrased differently like "supports this and that, >> similar to c++14 containers"? Or you're against any mention of it? >> > > Your rephrased suggestion is much better, imho. I'd be happy with that. > > >> >> Notably I guess, this is quite common already it seems.. >> - optional.h mentions its an implementation based on the class in c++17 >> - tuple.h mentions it's a clone of C++14 functionality >> - bind_internal.h mentions it has a copy of c++17 void_t >> - ptr_util.h points out MakeUnique is from c++14 >> - aligned_memory.h says that it is a substitute for functionality in >> c++17. >> > > Right, they all say "clone" or "replacement', but this doc here doesn't > read like that to me. (With your suggestion, it does.) > Ok thanks, that SGTM too! > > >> >> I can't measure the number of cases that could have but avoided >> mentioning future c++ specs though. >> >> >>> >>> >>>> It gives a really helpful tool to >>>> help understand how this works to compare to other things IMO. >>>> >>>> This is being added because it is useful, not because it is in c++14, >>>> but being done in a way that will be familiar with other c++14 >>>> containers. This allows you to key your set with a unique_ptr, instead >>>> of having to do flat_map<T*, unqiue_ptr<T>> for example, or avoid >>>> constructing temporary std::strings on every lookup. >>>> >>>> https://codereview.chromium.org/2944523002/ >>>> >>> >>> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
content/browser/devtools/protocol/target_auto_attacher.cc LGTM
The CQ bit was checked by dyaroshev@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
@danakj, @thakis Did I understand your suggestion correctly?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I think so, thanks, that LGTM
The CQ bit was checked by dyaroshev@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org, jdoerrie@chromium.org, thakis@chromium.org, sky@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2944523002/#ps380001 (title: "Rebase.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dyaroshev@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org, danakj@chromium.org, jdoerrie@chromium.org, thakis@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2944523002/#ps400001 (title: "Rebase 2.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dyaroshev@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org, danakj@chromium.org, jdoerrie@chromium.org, thakis@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2944523002/#ps420001 (title: "Other platforms compilation.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dyaroshev@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org, danakj@chromium.org, jdoerrie@chromium.org, thakis@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2944523002/#ps440001 (title: "Other platforms compilation 2.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by dyaroshev@yandex-team.ru
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
``` Patch blocked on closed tree (tree status: Tree is currently not open: Tree is closed (Automatic: "compile" on https://build.chromium.org/p/chromium.win/builders/WinClang64%20%28dbg%29/bui... "WinClang64 (dbg)" from 00043bdefebe1d854b0cffbd1d35d90c0091057c: lukasza@chromium.org) try job: just triggered) ``` Can somebody please merge this patch for me? This is how I feel about this patch: http://i.imgur.com/nLT4Kfl.gif
``` Patch blocked on closed tree (tree status: Tree is currently not open: Tree is closed (Automatic: "compile" on https://build.chromium.org/p/chromium.win/builders/WinClang64%20%28dbg%29/bui... "WinClang64 (dbg)" from 00043bdefebe1d854b0cffbd1d35d90c0091057c: lukasza@chromium.org) try job: just triggered) ``` Can somebody please merge this patch for me? This is how I feel about this patch: http://i.imgur.com/nLT4Kfl.gif
CQ is committing da patch.
Bot data: {"patchset_id": 440001, "attempt_start_ts": 1500738277230060,
"parent_rev": "d4de1c23d68363b5b9e78d31bb37b4ebbf881726", "commit_rev":
"0d31be9192093b5677941ac4280e0c8008167746"}
CQ is committing da patch.
Bot data: {"patchset_id": 440001, "attempt_start_ts": 1500738277230060,
"parent_rev": "8af4ef8b433ae41baae4cea697ad2ce66c530da2", "commit_rev":
"99a394d8dfee489d2a45308681f2bbbbf28ee6f8"}
Message was sent while issue was closed.
Description was changed from ========== Improving flat containers interface. This patch adds a few features to base::flat_set/base::flat_map. 1) C++14 overloads of search operations. Now count, find, equal_range, lower_bound, upper_bound accept any type comparable with key_type if the comparison is transparent. Comparison is considered to be transparent if it has `is_transparent` member typedef (http://en.cppreference.com/w/cpp/utility/functional/less_void) We removed non-templated versions, because they were mainly for backward compatibility in extremely rare cases. 2) Templated version of erase. This version was in the original proposal but later was removed as a result of a bug report, because it changed overload resolution for `erase(iterator)`. We provide an explicit `erase(iterator)` overload that addresses this issue. BUG=682254 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Improving flat containers interface. This patch adds a few features to base::flat_set/base::flat_map. 1) C++14 overloads of search operations. Now count, find, equal_range, lower_bound, upper_bound accept any type comparable with key_type if the comparison is transparent. Comparison is considered to be transparent if it has `is_transparent` member typedef (http://en.cppreference.com/w/cpp/utility/functional/less_void) We removed non-templated versions, because they were mainly for backward compatibility in extremely rare cases. 2) Templated version of erase. This version was in the original proposal but later was removed as a result of a bug report, because it changed overload resolution for `erase(iterator)`. We provide an explicit `erase(iterator)` overload that addresses this issue. BUG=682254 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2944523002 Cr-Commit-Position: refs/heads/master@{#488865} Committed: https://chromium.googlesource.com/chromium/src/+/99a394d8dfee489d2a45308681f2... ==========
Message was sent while issue was closed.
Committed patchset #22 (id:440001) as https://chromium.googlesource.com/chromium/src/+/99a394d8dfee489d2a45308681f2... |
