Use initializer_list and string_view in DohProviderEntry. [chromium/src : main]
Prashant Nevase (Gerrit)
Prashant Nevase voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Adam Rice
- John Lee
- Mayur Patil
- mmenke
- suresh potti
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 5 comments![]( "Open in Gerrit")
Thanks for the change.
One other thing:
DohProviderEntry::GetList() does a lot of calls to `new`, which is inefficient and bad practice.
Instead, could you change it to
```
static const base::NoDestructor<DohProviderEntry> array[] = {
{
"AlekBergNl",
MAKE_BASE_FEATURE_WITH_STATIC_STORAGE(
DohProviderAlekBergNl, base::FEATURE_ENABLED_BY_DEFAULT),
/*dns_over_53_server_ip_strs=*/{}, /*dns_over_tls_hostnames=*/{},
"https://dnsnl.alekberg.net/dns-query{?dns}",
/*ui_name=*/"alekberg.net (NL)",
/*privacy_policy=*/"https://alekberg.net/privacy",
/*display_globally=*/false,
/*display_countries=*/{"NL"}, LoggingLevel::kNormal
},
< rest of the entries >
};
static const base::NoDestructor<DohProviderEntry::List> providers =
base::ToVector(array,
[](const DohProviderEntry& entry) {
return raw_ptr<const DohProviderEntry, VectorExperimental>(&entry);
});
```
Ultimately I'd like to make `array` constexpr and eliminate most of the startup overhead completely, but make `ParseValidDohTemplate()` work on a `constexpr` context will be quite difficult and is out-of-scope for this CL.
std::string provider,It's weird that this is a std::string. This can also be a `std::string_view`, as can the member variable and the parameter to ConstructForTesting().
std::set<std::string> display_countries,`display_countries` can also be `base::flat_set<std::string_view>`.
std::initializer_list<std::string_view> dns_over_tls_hostnames;This should stay as a set, since it is used for lookups at runtime. However, this and the other std::sets should be base::flat_set instead because it is more space-efficient.
// Use DohProviderEntry in such a way that the lifetime of its members isI don't think we need this comment here, because production code can't construct a DohProviderEntry. Instead, comment that DohProviderEntries are only constructed with static data in production.
Then add a comment to ConstructForTesting() that the caller has to ensure that the parameters are static or outlive the object.
Related details
- John Lee
- Mayur Patil
- Prashant Nevase
- mmenke
- suresh potti
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mayur Patil (Gerrit)
Mayur Patil added 1 comment![]( "Open in Gerrit")
std::string provider,It's weird that this is a std::string. This can also be a `std::string_view`, as can the member variable and the parameter to ConstructForTesting().
If this is about std::string provider, cannot mark it as string_view because, it is a member variable for this struct. As well if you checked its usage of GetDohProviderIdForHistogramFromNameserver at
https://source.chromium.org/chromium/chromium/src/+/main:net/dns/dns_util.cc;drc=60130be50f68c89220d8ace62c87155025267a2c;bpv=1;bpt=1;l=192?q=doh&ss=chromium
it could cause dangling pointer error.
There might be need to mark "Other" string as static const to make it possible. But still need to recheck thorougly
Related details
- Adam Rice
- John Lee
Mayur Patil (Gerrit)
Mayur Patil added 2 comments![]( "Open in Gerrit")
std::initializer_list<std::string_view> dns_over_tls_hostnames;This should stay as a set, since it is used for lookups at runtime. However, this and the other std::sets should be base::flat_set instead because it is more space-efficient.
Acknowledged
// Use DohProviderEntry in such a way that the lifetime of its members isI don't think we need this comment here, because production code can't construct a DohProviderEntry. Instead, comment that DohProviderEntries are only constructed with static data in production.
Then add a comment to ConstructForTesting() that the caller has to ensure that the parameters are static or outlive the object.
Acknowledged
Related details
- Adam Rice
- John Lee
- Prashant Nevase
- mmenke
- suresh potti
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
mmenke (Gerrit)
mmenke added 5 comments![]( "Open in Gerrit")
Removing myself as a reviewer - I don't think this CL needs 5 reviewers. Deferring to Adam.
// the dropdown menu.
// DohProviderEntries are only constructed with static data in production.I think you need to explain what that means, both here and in the constructor. Namely, they must be constructed with strings with static storage duration, that are never destroyed.
std::move(ui_name), std::move(privacy_policy), display_globally,std::move is generally not called on string_views, since it's just as fast to copy them.
ip_addresses(ParseIPs(dns_over_53_server_ip_strs)),
dns_over_tls_hostnames(std::move(dns_over_tls_hostnames)),std::move() doesn't do anything here, either, does it? Not that familiar with initializer lists.
ui_name(std::move(ui_name)),
privacy_policy(std::move(privacy_policy)),std::move() here doesn't do anything, since we're constructing a string.
Related details
- Adam Rice
- John Lee
- Mayur Patil
- Prashant Nevase
- suresh potti
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 3 comments![]( "Open in Gerrit")
std::string provider,Mayur PatilIt's weird that this is a std::string. This can also be a `std::string_view`, as can the member variable and the parameter to ConstructForTesting().
If this is about std::string provider, cannot mark it as string_view because, it is a member variable for this struct. As well if you checked its usage of GetDohProviderIdForHistogramFromNameserver at
https://source.chromium.org/chromium/chromium/src/+/main:net/dns/dns_util.cc;drc=60130be50f68c89220d8ace62c87155025267a2c;bpv=1;bpt=1;l=192?q=doh&ss=chromiumit could cause dangling pointer error.
There might be need to mark "Other" string as static const to make it possible. But still need to recheck thorougly
Actually, it's fine for GetDohProviderIdForHistogramFromNameserver() to return std::string even if `entries[0]->provider` is a std::string_view as it could be written as
```
return entries.empty() ? "Other" : std::string(entries[0]->provider);
```
However, I think a better solution is to make that function return `std::string_view`. It's perfectly safe to convert an ordinary string literal to a `std::string_view` as it will be stored in the read-only static data part of the binary, so there's no risk of a dangling pointer.
So, yes, I think the member variable should change type to `std::string_view`.
std::move(ui_name), std::move(privacy_policy), display_globally,std::move is generally not called on string_views, since it's just as fast to copy them.
Yes. It's better not to use std::move where it does nothing, as it slows down the reader as they wonder why it is there.
ip_addresses(ParseIPs(dns_over_53_server_ip_strs)),
dns_over_tls_hostnames(std::move(dns_over_tls_hostnames)),std::move() doesn't do anything here, either, does it? Not that familiar with initializer lists.
`dns_over_tls_hostnames` is a `base::flat_set` passed by value in the newest patch set, so std::move is correct here. You were right that it didn't make sense when it was an `initializer_list`, as `std::initializer_list<std::string_view>` is internally similar to `std::span<const std::string_view>`.
Related details
- John Lee
- Mayur Patil
- Prashant Nevase
- suresh potti
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mayur Patil (Gerrit)
Mayur Patil added 6 comments![]( "Open in Gerrit")
Thanks for the change.
One other thing:
DohProviderEntry::GetList() does a lot of calls to `new`, which is inefficient and bad practice.
Instead, could you change it to
```
static const base::NoDestructor<DohProviderEntry> array[] = {
{
"AlekBergNl",
MAKE_BASE_FEATURE_WITH_STATIC_STORAGE(
DohProviderAlekBergNl, base::FEATURE_ENABLED_BY_DEFAULT),
/*dns_over_53_server_ip_strs=*/{}, /*dns_over_tls_hostnames=*/{},
"https://dnsnl.alekberg.net/dns-query{?dns}",
/*ui_name=*/"alekberg.net (NL)",
/*privacy_policy=*/"https://alekberg.net/privacy",
/*display_globally=*/false,
/*display_countries=*/{"NL"}, LoggingLevel::kNormal
},
< rest of the entries >
};
static const base::NoDestructor<DohProviderEntry::List> providers =
base::ToVector(array,
[](const DohProviderEntry& entry) {
return raw_ptr<const DohProviderEntry, VectorExperimental>(&entry);
});
```Ultimately I'd like to make `array` constexpr and eliminate most of the startup overhead completely, but make `ParseValidDohTemplate()` work on a `constexpr` context will be quite difficult and is out-of-scope for this CL.
I tried above suggestions but I keep getting errors. could you suggest any work arounds for this problem or we can take it separately in other PR.... (Also Enable move and copy constructors)
Error log:
../../net/dns/public/doh_provider_entry.cc(71,61): error: no matching constructor for initialization of 'const base::NoDestructor<DohProviderEntry>'
71 | static const base::NoDestructor<DohProviderEntry> a2[] = {{
| ^
72 | "AlekBergNl",
| ~~~~~~~~~~~~~
73 | MAKE_BASE_FEATURE_WITH_STATIC_STORAGE(
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
74 | DohProviderAlekBergNl, base::FEATURE_ENABLED_BY_DEFAULT),
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
75 | /*dns_over_53_server_ip_strs=*/{}, /*dns_over_tls_hostnames=*/{},
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
76 | "https://dnsnl.alekberg.net/dns-query{?dns}",
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
77 | /*ui_name=*/"alekberg.net (NL)",
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
78 | /*privacy_policy=*/"https://alekberg.net/privacy",
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
79 | /*display_globally=*/false,
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~
80 | /*display_countries=*/{"NL"}, LoggingLevel::kNormal},
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../..\base/no_destructor.h(98,12): note: candidate template ignored: substitution failure: deduced incomplete pack <const char (&)[11], const base::Feature *, (no value), (no value), const char (&)[43], const char (&)[18], const char (&)[29], _Bool, (no value), net::DohProviderEntry::LoggingLevel> for template parameter 'Args'
97 | template <typename... Args>
| ~~~~
98 | explicit NoDestructor(Args&&... args) {
| ^
../..\base/no_destructor.h(104,12): note: candidate constructor not viable: requires single argument 'x', but 10 arguments were provided
104 | explicit NoDestructor(const T& x) { new (storage_) T(x); }
| ^ ~~~~~~~~~~
../..\base/no_destructor.h(105,12): note: candidate constructor not viable: requires single argument 'x', but 10 arguments were provided
105 | explicit NoDestructor(T&& x) { new (storage_) T(std::move(x)); }
| ^ ~~~~~
../..\base/no_destructor.h(107,3): note: candidate constructor not viable: requires 1 argument, but 10 were provided
107 | NoDestructor(const NoDestructor&) = delete;
| ^ ~~~~~~~~~~~~~~~~~~~
../../net/dns/public/doh_provider_entry.cc(81,11): error: no matching constructor for initialization of 'const base::NoDestructor<DohProviderEntry>'
81 | {
| ^
82 | "AlekBergN2",
| ~~~~~~~~~~~~~
83 | MAKE_BASE_FEATURE_WITH_STATIC_STORAGE(
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
84 | DohProviderAlekBergNl, base::FEATURE_ENABLED_BY_DEFAULT),
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
85 | /*dns_over_53_server_ip_strs=*/{}, /*dns_over_tls_hostnames=*/{},
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
86 | "https://dnsnl.alekberg.net/dns-query{?dns}",
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
87 | /*ui_name=*/"alekberg.net (NL)",
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
88 | /*privacy_policy=*/"https://alekberg.net/privacy",
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
89 | /*display_globally=*/false,
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~
90 | /*display_countries=*/{"NL"}, LoggingLevel::kNormal}};
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../..\base/no_destructor.h(98,12): note: candidate template ignored: substitution failure: deduced incomplete pack <const char (&)[11], const base::Feature *, (no value), (no value), const char (&)[43], const char (&)[18], const char (&)[29], _Bool, (no value), net::DohProviderEntry::LoggingLevel> for template parameter 'Args'
97 | template <typename... Args>
| ~~~~
98 | explicit NoDestructor(Args&&... args) {
| ^
../..\base/no_destructor.h(104,12): note: candidate constructor not viable: requires single argument 'x', but 10 arguments were provided
104 | explicit NoDestructor(const T& x) { new (storage_) T(x); }
| ^ ~~~~~~~~~~
../..\base/no_destructor.h(105,12): note: candidate constructor not viable: requires single argument 'x', but 10 arguments were provided
105 | explicit NoDestructor(T&& x) { new (storage_) T(std::move(x)); }
| ^ ~~~~~
../..\base/no_destructor.h(107,3): note: candidate constructor not viable: requires 1 argument, but 10 were provided
107 | NoDestructor(const NoDestructor&) = delete;
----------------------------------------------------------------------------
I was able to build normal array of objects, with copy constructor and move constructors which where originally deleted by making them default.
DohProviderEntry::DohProviderEntry(DohProviderEntry& other) = default;
DohProviderEntry::DohProviderEntry(DohProviderEntry&& other) = default;
const DohProviderEntry arr[] = {{
"AlekBergNl",
MAKE_BASE_FEATURE_WITH_STATIC_STORAGE(
DohProviderAlekBergNl, base::FEATURE_ENABLED_BY_DEFAULT),
/*dns_over_53_server_ip_strs=*/{}, /*dns_over_tls_hostnames=*/{},
"https://dnsnl.alekberg.net/dns-query{?dns}",
/*ui_name=*/"alekberg.net (NL)",
/*privacy_policy=*/"https://alekberg.net/privacy",
/*display_globally=*/false,
/*display_countries=*/{"NL"}, LoggingLevel::kNormal},
{
"AlekBergN2",
MAKE_BASE_FEATURE_WITH_STATIC_STORAGE(
DohProviderAlekBergNl, base::FEATURE_ENABLED_BY_DEFAULT),
/*dns_over_53_server_ip_strs=*/{}, /*dns_over_tls_hostnames=*/{},
"https://dnsnl.alekberg.net/dns-query{?dns}",
/*ui_name=*/"alekberg.net (NL)",
/*privacy_policy=*/"https://alekberg.net/privacy",
/*display_globally=*/false,
/*display_countries=*/{"NL"}, LoggingLevel::kNormal}};
for static array it failed because of non trivial constructor, which can be fixed by defining it explicitly
std::string provider,Mayur PatilIt's weird that this is a std::string. This can also be a `std::string_view`, as can the member variable and the parameter to ConstructForTesting().
Adam RiceIf this is about std::string provider, cannot mark it as string_view because, it is a member variable for this struct. As well if you checked its usage of GetDohProviderIdForHistogramFromNameserver at
https://source.chromium.org/chromium/chromium/src/+/main:net/dns/dns_util.cc;drc=60130be50f68c89220d8ace62c87155025267a2c;bpv=1;bpt=1;l=192?q=doh&ss=chromiumit could cause dangling pointer error.
There might be need to mark "Other" string as static const to make it possible. But still need to recheck thorougly
Actually, it's fine for GetDohProviderIdForHistogramFromNameserver() to return std::string even if `entries[0]->provider` is a std::string_view as it could be written as
```
return entries.empty() ? "Other" : std::string(entries[0]->provider);
```However, I think a better solution is to make that function return `std::string_view`. It's perfectly safe to convert an ordinary string literal to a `std::string_view` as it will be stored in the read-only static data part of the binary, so there's no risk of a dangling pointer.
So, yes, I think the member variable should change type to `std::string_view`.
Acknowledged
`display_countries` can also be `base::flat_set<std::string_view>`.
Acknowledged
std::move(ui_name), std::move(privacy_policy), display_globally,Adam Ricestd::move is generally not called on string_views, since it's just as fast to copy them.
Yes. It's better not to use std::move where it does nothing, as it slows down the reader as they wonder why it is there.
Acknowledged
ip_addresses(ParseIPs(dns_over_53_server_ip_strs)),
dns_over_tls_hostnames(std::move(dns_over_tls_hostnames)),Adam Ricestd::move() doesn't do anything here, either, does it? Not that familiar with initializer lists.
`dns_over_tls_hostnames` is a `base::flat_set` passed by value in the newest patch set, so std::move is correct here. You were right that it didn't make sense when it was an `initializer_list`, as `std::initializer_list<std::string_view>` is internally similar to `std::span<const std::string_view>`.
Acknowledged
ui_name(std::move(ui_name)),
privacy_policy(std::move(privacy_policy)),std::move() here doesn't do anything, since we're constructing a string.
- Adam Rice
- John Lee
- Prashant Nevase
- suresh potti
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mayur Patil (Gerrit)
Mayur Patil added 1 comment![]( "Open in Gerrit")
// the dropdown menu.
// DohProviderEntries are only constructed with static data in production.I think you need to explain what that means, both here and in the constructor. Namely, they must be constructed with strings with static storage duration, that are never destroyed.
Acknowledged
Adam Rice (Gerrit)
Adam Rice added 1 comment![]( "Open in Gerrit")
Sorry about that. I forgot that using base::NoDestructor would prevent type inference for the array. There are a number of ways around this, but probably the least-worst is to make explicit calls to the `DohProviderEntry` constructor, like this:
```
static const base::NoDestructor<DohProviderEntry> array[] = {
DohProviderEntry("AlekBergNl",
MAKE_BASE_FEATURE_WITH_STATIC_STORAGE(
DohProviderAlekBergNl, base::FEATURE_ENABLED_BY_DEFAULT),
/*dns_over_53_server_ip_strs=*/{}, /*dns_over_tls_hostnames=*/{},
"https://dnsnl.alekberg.net/dns-query{?dns}",
/*ui_name=*/"alekberg.net (NL)",
/*privacy_policy=*/"https://alekberg.net/privacy",
/*display_globally=*/false,
/*display_countries=*/{"NL"}, LoggingLevel::kNormal),
DohProviderEntry("the next one", ...),
< rest of the entries >
};
```
You will need the move constructor for DohProviderEntry to be available in order for this to compile. I think the copy constructor is unnecessary.Since this is the only code outside of tests that creates DohProviderEntry objects, and they are always const, no other code will be able to call the move constructor anyway, so it should be harmless.
I also made a mistake with the lambda that is used in base::ToVector. It should have been
```
static const base::NoDestructor<DohProviderEntry::List> providers =
base::ToVector(array,
[](const base::NoDestructor<DohProviderEntry>& entry) {
return raw_ptr<const DohProviderEntry, VectorExperimental>(entry.get());
});
```Sorry I wasted your time with my errors.
Related details
- John Lee
- Mayur Patil
- Prashant Nevase
- suresh potti
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mayur Patil (Gerrit)
Mayur Patil added 1 comment![]( "Open in Gerrit")
Creation of DohProviderEntry objects for was not working the way it is mentioned.
I had to create base::NoDestructor<DohProviderEntry>(...) objects array.
static const base::NoDestructor<DohProviderEntry> array[] = {
base::NoDestructor<DohProviderEntry>("AlekBergNl",
MAKE_BASE_FEATURE_WITH_STATIC_STORAGE(
DohProviderAlekBergNl, base::FEATURE_ENABLED_BY_DEFAULT),
/*dns_over_53_server_ip_strs=*/{}, /*dns_over_tls_hostnames=*/{},
"https://dnsnl.alekberg.net/dns-query{?dns}",
/*ui_name=*/"alekberg.net (NL)",
....)
<ENTRIES>
}
Also base::Vector conversions mentioned fails giving error as
no viable conversion from 'std::vector<ProjectedType>' (aka 'vector<base::raw_ptr<const net::DohProviderEntry, partition_alloc::internal::RawPtrTraits::kMayDangle>>') to 'const base::NoDestructor<DohProviderEntry::List>' (aka 'const NoDestructor<vector<raw_ptr<const DohProviderEntry, VectorExperimental>>>')
so I had to first create temp vector and then cast it to base::NoDestructor<T>
instance...
static const base::NoDestructor<DohProviderEntry::List> providers = []() {
const DohProviderEntry::List temp_vector = base::ToVector(
array, [](const base::NoDestructor<DohProviderEntry>& entry) {
return raw_ptr<const DohProviderEntry, VectorExperimental>(
entry.get());
});
return base::NoDestructor<DohProviderEntry::List>(temp_vector);
}();
There is one more easy to read solution but I did not find any usage or references in chromium source code directly. It is used in few third party libraries
The only implications of these macros will be that it is tied to clang compiler
[[clang::always_destroy]] static const DohProviderEntry array[] = {{
"AlekBergNl",
MAKE_BASE_FEATURE_WITH_STATIC_STORAGE(
DohProviderAlekBergNl, base::FEATURE_ENABLED_BY_DEFAULT),
/*dns_over_53_server_ip_strs=*/{}, /*dns_over_tls_hostnames=*/{},
"https://dnsnl.alekberg.net/dns-query{?dns}",
/*ui_name=*/"alekberg.net (NL)",
/*privacy_policy=*/"https://alekberg.net/privacy",
/*display_globally=*/false,
/*display_countries=*/{"NL"}, LoggingLevel::kNormal},
{
"CleanBrowsingAdult",
MAKE_BASE_FEATURE_WITH_STATIC_STORAGE(
DohProviderCleanBrowsingAdult, base::FEATURE_ENABLED_BY_DEFAULT),
{"185.228.168.10", "185.228.169.11", "2a0d:2a00:1::1",
"2a0d:2a00:2::1"},
/*dns_over_tls_hostnames=*/{"adult-filter-dns.cleanbrowsing.org"},
"https://doh.cleanbrowsing.org/doh/adult-filter{?dns}",
/*ui_name=*/"", /*privacy_policy=*/"",
/*display_globally=*/false, /*display_countries=*/{},
LoggingLevel::kNormal
}}static const base::NoDestructor<DohProviderEntry::List> providers
(base::ToVector(array, [](const DohProviderEntry& entry) {
return raw_ptr<const DohProviderEntry, VectorExperimental>(&entry);
}));
If objects are not required to be destroyed even after process completes in that case [[clang::no_destroy]] can be used.
Related details
- Adam Rice
- John Lee
- Prashant Nevase
- suresh potti
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mayur Patil (Gerrit)
seems NoDestructor<DohProviderEntry> is getting destroyed on run.
Mayur Patil (Gerrit)
check link :
https://godbolt.org/z/e65Tfv5M5
Related details
- Adam Rice
- John Lee
- Prashant Nevase
- suresh potti
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 1 comment![]( "Open in Gerrit")
Sorry, about that. I found an answer, but it was really hard to get working: https://godbolt.org/z/rPMhYGbaP.
Related details
- John Lee
- Mayur Patil
- Prashant Nevase
- suresh potti
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mayur Patil (Gerrit)
Mayur Patil added 1 comment![]( "Open in Gerrit")
- Adam Rice
- John Lee
- Prashant Nevase
- suresh potti
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mayur Patil (Gerrit)
Mayur Patil added 1 comment![]( "Open in Gerrit")
It worked and associated issues are resolved
Related details
- Adam Rice
- John Lee
- Prashant Nevase
- suresh potti
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice voted and added 4 comments![]( "Open in Gerrit")
Votes added by Adam Rice
| Code-Review | +1 |
4 comments
std::move(provider), feature, std::move(dns_over_53_server_ip_strs),std::move() should not be used with `provider`, `dns_over_53_server_ip_strs`, `ui_name`, and `privacy_policy` as `string_view` and `initializer_list` objects are not usefully movable because they do not own the data they refer to.
std::move(dns_over_https_server_ip_strs))),std::move() should not be used with `dns_over_https_server_ip_strs` as it is an `initializer_list`.
DohProviderEntry::DohProviderEntry(DohProviderEntry&& other)
: provider(std::move(other.provider)),
feature(other.feature),
ip_addresses(std::move(other.ip_addresses)),
dns_over_tls_hostnames(std::move(other.dns_over_tls_hostnames)),
doh_server_config(std::move(other.doh_server_config)),
ui_name(std::move(other.ui_name)),
privacy_policy(std::move(other.privacy_policy)),
display_globally(other.display_globally),
display_countries(std::move(other.display_countries)),
logging_level(other.logging_level) {}I think you should be able to just `= default` this one.
```suggestion
DohProviderEntry::DohProviderEntry(DohProviderEntry&& other) = default;
- John Lee
- Mayur Patil
- Prashant Nevase
- suresh potti
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mayur Patil (Gerrit)
Mayur Patil added 3 comments![]( "Open in Gerrit")
std::move(provider), feature, std::move(dns_over_53_server_ip_strs),std::move() should not be used with `provider`, `dns_over_53_server_ip_strs`, `ui_name`, and `privacy_policy` as `string_view` and `initializer_list` objects are not usefully movable because they do not own the data they refer to.
Acknowledged
std::move() should not be used with `dns_over_https_server_ip_strs` as it is an `initializer_list`.
Acknowledged
DohProviderEntry::DohProviderEntry(DohProviderEntry&& other)
: provider(std::move(other.provider)),
feature(other.feature),
ip_addresses(std::move(other.ip_addresses)),
dns_over_tls_hostnames(std::move(other.dns_over_tls_hostnames)),
doh_server_config(std::move(other.doh_server_config)),
ui_name(std::move(other.ui_name)),
privacy_policy(std::move(other.privacy_policy)),
display_globally(other.display_globally),
display_countries(std::move(other.display_countries)),
logging_level(other.logging_level) {}I think you should be able to just `= default` this one.
```suggestion
DohProviderEntry::DohProviderEntry(DohProviderEntry&& other) = default;```
- Adam Rice
- John Lee
- Prashant Nevase
- suresh potti
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice voted and added 1 comment![]( "Open in Gerrit")
Votes added by Adam Rice
| Code-Review | +1 |
1 comment
My +1 seems to have vanished. Adding it back.
Related details
- John Lee
- Mayur Patil
- Prashant Nevase
- suresh potti
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Prashant Nevase (Gerrit)
Prashant Nevase added 2 comments![]( "Open in Gerrit")
base::flat_set<std::string_view> dns_over_tls_hostnames,Can you mention why this is changed to flat_set in cl description.
static const base::NoDestructor<DohProviderEntry::List> providers(Can you add blank line before this?
Related details
- John Lee
- Mayur Patil
- suresh potti
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Prashant Nevase (Gerrit)
Prashant Nevase voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- John Lee
- Mayur Patil
- suresh potti
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
suresh potti (Gerrit)
suresh potti added 3 comments![]( "Open in Gerrit")
for (const DohProviderEntry* entry : DohProviderEntry::GetList()) {nit : const auto*
for (const DohProviderEntry* entry : DohProviderEntry::GetList()) {nit : const auto*
for (const std::string_view& s : entry->dns_over_tls_hostnames) {nit : const auto&
Related details
- John Lee
- Mayur Patil
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mayur Patil (Gerrit)
Mayur Patil added 6 comments![]( "Open in Gerrit")
base::flat_set<std::string_view> dns_over_tls_hostnames,Can you mention why this is changed to flat_set in cl description.
Acknowledged
static const base::NoDestructor<DohProviderEntry::List> providers(Can you add blank line before this?
Done
for (const DohProviderEntry* entry : DohProviderEntry::GetList()) {Mayur Patilnit : const auto*
cant use auto* because GetList store array vector of raw_ptr of DohProviderEntry.
error: variable 'entry' with type 'const auto *' has incompatible initializer of type 'const base::raw_ptr<const net::DohProviderEntry, partition_alloc::internal::RawPtrTraits::kMayDangle>'
24 | for (const auto* entry : DohProviderEntry::GetList()) {
for (const DohProviderEntry* entry : DohProviderEntry::GetList()) {Mayur Patilnit : const auto*
cant use auto* because GetList store array vector of raw_ptr of DohProviderEntry.
error: variable 'entry' with type 'const auto *' has incompatible initializer of type 'const base::raw_ptr<const net::DohProviderEntry, partition_alloc::internal::RawPtrTraits::kMayDangle>'
24 | for (const auto* entry : DohProviderEntry::GetList()) {
for (const std::string_view& s : entry->dns_over_tls_hostnames) {Mayur Patilnit : const auto&
Done
for (const std::string_view& s : entry->dns_over_tls_hostnames) {- Adam Rice
- John Lee
- Prashant Nevase
- suresh potti
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice voted and added 1 comment![]( "Open in Gerrit")
Votes added by Adam Rice
| Code-Review | +1 |
- John Lee
- Mayur Patil
- Prashant Nevase
- suresh potti
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rebekah Potter (Gerrit)
Rebekah Potter voted and added 1 comment![]( "Open in Gerrit")
Votes added by Rebekah Potter
| Code-Review | +1 |
1 comment
lgtm for settings_secure_dns_handler_browsertest.cc
Related details
- John Lee
- Mayur Patil
- Prashant Nevase
- suresh potti
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John Lee (Gerrit)
John Lee voted Code-Review+1![]( "Open in Gerrit")
- Mayur Patil
- Prashant Nevase
- suresh potti
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mayur Patil (Gerrit)
Mayur Patil voted Commit-Queue+2![]( "Open in Gerrit")
- Prashant Nevase
- suresh potti
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Use initializer_list and string_view in DohProviderEntry.
This change updates string to string_view and replaces std::set with
std::initializer_list in the DohProviderEntry files where needed.Also
added base::flat_set to improve memory efficiencies.We use
std::initializer_list because it is only required for the constructor and looping.
- M chrome/browser/ui/webui/settings/settings_secure_dns_handler_browsertest.cc
- M net/dns/dns_util.cc
- M net/dns/public/doh_provider_entry.cc
- M net/dns/public/doh_provider_entry.h
- M net/dns/public/doh_provider_entry_unittest.cc
Code-Review: +1 by Adam Rice, +1 by Rebekah Potter, +1 by John Lee
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |