You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/11/13 00:03:11 UTC
[GitHub] [incubator-kvrocks] MaximSmolskiy opened a new pull request, #1112: Fix modernize-loop-convert warning reported by clang-tidy
MaximSmolskiy opened a new pull request, #1112:
URL: https://github.com/apache/incubator-kvrocks/pull/1112
Fix #1095
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1112: Fix modernize-loop-convert warning reported by clang-tidy
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1112:
URL: https://github.com/apache/incubator-kvrocks/pull/1112#discussion_r1020909038
##########
src/types/redis_string.cc:
##########
@@ -137,8 +137,8 @@ std::vector<rocksdb::Status> String::MGet(const std::vector<Slice> &keys, std::v
// don't use range-based for loop here, coz the slice member
// would refer the address instead of copy the value, and use
// range-based for loop may cause all members refer to the same addr
- for (size_t i = 0; i < ns_keys.size(); i++) {
- slice_keys.emplace_back(ns_keys[i]);
+ for (const auto &ns_key : ns_keys) {
Review Comment:
`emplace_back` will invalidate all references and iterators (if capacity changes), so it is not allowed to use an iterator or reference based loop (as well as for-range) here. Of cource, index is always safe.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1112: Fix modernize-loop-convert warning reported by clang-tidy
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1112:
URL: https://github.com/apache/incubator-kvrocks/pull/1112#discussion_r1021223956
##########
src/types/redis_string.cc:
##########
@@ -137,8 +137,8 @@ std::vector<rocksdb::Status> String::MGet(const std::vector<Slice> &keys, std::v
// don't use range-based for loop here, coz the slice member
// would refer the address instead of copy the value, and use
// range-based for loop may cause all members refer to the same addr
- for (size_t i = 0; i < ns_keys.size(); i++) {
- slice_keys.emplace_back(ns_keys[i]);
+ for (const auto &ns_key : ns_keys) {
Review Comment:
@PragmaTwice @torwig @MaximSmolskiy If my memory serves, the root cause is the for loop would reuse the same variable `ns_key` address if we use `for (auto ns_key : ns_keys)` instead of `for (auto &ns_key : ns_keys)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1112: Fix modernize-loop-convert warning reported by clang-tidy
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1112:
URL: https://github.com/apache/incubator-kvrocks/pull/1112#discussion_r1020910827
##########
src/types/redis_string.cc:
##########
@@ -137,8 +137,8 @@ std::vector<rocksdb::Status> String::MGet(const std::vector<Slice> &keys, std::v
// don't use range-based for loop here, coz the slice member
// would refer the address instead of copy the value, and use
// range-based for loop may cause all members refer to the same addr
- for (size_t i = 0; i < ns_keys.size(); i++) {
- slice_keys.emplace_back(ns_keys[i]);
+ for (const auto &ns_key : ns_keys) {
Review Comment:
Oh sorry. I did not notice that there are two different vectors. Some time to wait me to understand these code again :rofl:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1112: Fix modernize-loop-convert warning reported by clang-tidy
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1112:
URL: https://github.com/apache/incubator-kvrocks/pull/1112#discussion_r1021223956
##########
src/types/redis_string.cc:
##########
@@ -137,8 +137,8 @@ std::vector<rocksdb::Status> String::MGet(const std::vector<Slice> &keys, std::v
// don't use range-based for loop here, coz the slice member
// would refer the address instead of copy the value, and use
// range-based for loop may cause all members refer to the same addr
- for (size_t i = 0; i < ns_keys.size(); i++) {
- slice_keys.emplace_back(ns_keys[i]);
+ for (const auto &ns_key : ns_keys) {
Review Comment:
@PragmaTwice @torwig @MaximSmolskiy If my memory serves, the root cause is the for loop would reuse the same variable `ns_key` address if we use `for (auto ns_key : ns_keys)` instead of `for (auto &ns_key : ns_keys)`.
So we can remove this comment if `for (auto &ns_key : ns_keys)` works well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #1112: Fix modernize-loop-convert warning reported by clang-tidy
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #1112:
URL: https://github.com/apache/incubator-kvrocks/pull/1112#discussion_r1020913956
##########
src/types/redis_string.cc:
##########
@@ -137,8 +137,8 @@ std::vector<rocksdb::Status> String::MGet(const std::vector<Slice> &keys, std::v
// don't use range-based for loop here, coz the slice member
// would refer the address instead of copy the value, and use
// range-based for loop may cause all members refer to the same addr
- for (size_t i = 0; i < ns_keys.size(); i++) {
- slice_keys.emplace_back(ns_keys[i]);
+ for (const auto &ns_key : ns_keys) {
Review Comment:
I've just tested the following code:
```
std::vector<std::string> ns_keys = {"one", "two", "three", "four"};
std::vector<rocksdb::Slice> slice_keys;
for (size_t i = 0; i < ns_keys.size(); i++) {
slice_keys.emplace_back(ns_keys[i]);
}
for (auto key : slice_keys) {
std::cout << key.ToString() << std::endl;
}
```
And the output is:
```
one
two
three
four
```
Then I changed it to (as in the `clang` recommendation):
```
std::vector<std::string> ns_keys = {"one", "two", "three", "four"};
std::vector<rocksdb::Slice> slice_keys;
for (auto &ns_key : ns_keys) {
slice_keys.emplace_back(ns_key);
}
for (auto key : slice_keys) {
std::cout << key.ToString() << std::endl;
}
```
And the output is still the same.
Perhaps at some point in time, the code in `kvrocks` was changed but the comment is still here (an doutdated). Or there is something else.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #1112: Fix modernize-loop-convert warning reported by clang-tidy
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #1112:
URL: https://github.com/apache/incubator-kvrocks/pull/1112#discussion_r1020873024
##########
src/types/redis_string.cc:
##########
@@ -137,8 +137,8 @@ std::vector<rocksdb::Status> String::MGet(const std::vector<Slice> &keys, std::v
// don't use range-based for loop here, coz the slice member
// would refer the address instead of copy the value, and use
// range-based for loop may cause all members refer to the same addr
- for (size_t i = 0; i < ns_keys.size(); i++) {
- slice_keys.emplace_back(ns_keys[i]);
+ for (const auto &ns_key : ns_keys) {
Review Comment:
@MaximSmolskiy This place is really tricky. The comment above it explains why we should NOT use a range-based loop here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #1112: Fix modernize-loop-convert warning reported by clang-tidy
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #1112:
URL: https://github.com/apache/incubator-kvrocks/pull/1112#issuecomment-1313285634
I think we can still use range-based for loop and remove these comment to continue with this PR if it is confirmed that these comments are out-of-date and not correct now.
cc @git-hulk
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1112: Fix modernize-loop-convert warning reported by clang-tidy
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1112:
URL: https://github.com/apache/incubator-kvrocks/pull/1112#discussion_r1020912705
##########
src/types/redis_string.cc:
##########
@@ -137,8 +137,8 @@ std::vector<rocksdb::Status> String::MGet(const std::vector<Slice> &keys, std::v
// don't use range-based for loop here, coz the slice member
// would refer the address instead of copy the value, and use
// range-based for loop may cause all members refer to the same addr
- for (size_t i = 0; i < ns_keys.size(); i++) {
- slice_keys.emplace_back(ns_keys[i]);
+ for (const auto &ns_key : ns_keys) {
Review Comment:
Actually I cannot understand the comment. `Slice` will never *copy* a string, it will only *take reference to* a string. cc @git-hulk @ShooterIT
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice merged pull request #1112: Fix modernize-loop-convert warning reported by clang-tidy
Posted by GitBox <gi...@apache.org>.
PragmaTwice merged PR #1112:
URL: https://github.com/apache/incubator-kvrocks/pull/1112
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] MaximSmolskiy commented on a diff in pull request #1112: Fix modernize-loop-convert warning reported by clang-tidy
Posted by GitBox <gi...@apache.org>.
MaximSmolskiy commented on code in PR #1112:
URL: https://github.com/apache/incubator-kvrocks/pull/1112#discussion_r1020910263
##########
src/types/redis_string.cc:
##########
@@ -137,8 +137,8 @@ std::vector<rocksdb::Status> String::MGet(const std::vector<Slice> &keys, std::v
// don't use range-based for loop here, coz the slice member
// would refer the address instead of copy the value, and use
// range-based for loop may cause all members refer to the same addr
- for (size_t i = 0; i < ns_keys.size(); i++) {
- slice_keys.emplace_back(ns_keys[i]);
+ for (const auto &ns_key : ns_keys) {
Review Comment:
@PragmaTwice I'm sorry, but I still don't understand what the problem is. Here we iterate through vector `ns_keys` and put its values to another vector `slice_keys`. So, `ns_keys` shouldn't be updated during this loop. So, range-based for loop should be equivalent to index based loop here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1112: Fix modernize-loop-convert warning reported by clang-tidy
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1112:
URL: https://github.com/apache/incubator-kvrocks/pull/1112#discussion_r1020924757
##########
src/types/redis_string.cc:
##########
@@ -137,8 +137,8 @@ std::vector<rocksdb::Status> String::MGet(const std::vector<Slice> &keys, std::v
// don't use range-based for loop here, coz the slice member
// would refer the address instead of copy the value, and use
// range-based for loop may cause all members refer to the same addr
- for (size_t i = 0; i < ns_keys.size(); i++) {
- slice_keys.emplace_back(ns_keys[i]);
+ for (const auto &ns_key : ns_keys) {
Review Comment:
I think it can be something like:
```
std::vector<rocksdb::Slice> slice_keys(ns_keys.size());
std::copy(ns_keys.begin(), ns_keys.end(), slice_keys.begin());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #1112: Fix modernize-loop-convert warning reported by clang-tidy
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #1112:
URL: https://github.com/apache/incubator-kvrocks/pull/1112#issuecomment-1316269693
Thanks for your contribution!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org