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/06/25 15:45:02 UTC
[GitHub] [incubator-kvrocks] jackwener opened a new pull request, #672: enhance: use unique_ptr in Config
jackwener opened a new pull request, #672:
URL: https://github.com/apache/incubator-kvrocks/pull/672
Part of #663
In config.cc, use unique_ptr to eliminate some trivial manual deallocation
I have use `emplace_back` place the vector [List-initialization](https://en.cppreference.com/w/cpp/language/list_initialization).
Because it will use `copy constructor`, it will call `FieldWrapper(const FieldWrapper &wrapper)`.
So, I use `emplace_back`
--
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 #672: enhance: use unique_ptr in Config
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #672:
URL: https://github.com/apache/incubator-kvrocks/pull/672#discussion_r906750151
##########
src/config.cc:
##########
@@ -484,9 +488,6 @@ void Config::initFieldCallback() {
}
Config::~Config() {
Review Comment:
Could you delete it and add `~Config() = default;` in `config.h`?
We'd better make sure these trivial function in header to let compilers do more optimization like inlining and call elimination.
--
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 #672: enhance: use unique_ptr in Config
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #672:
URL: https://github.com/apache/incubator-kvrocks/pull/672#issuecomment-1166561445
Thanks for you contribution! Merging...
--
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] tisonkun commented on a diff in pull request #672: enhance: use unique_ptr in Config
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #672:
URL: https://github.com/apache/incubator-kvrocks/pull/672#discussion_r906786223
##########
src/config.cc:
##########
@@ -84,11 +84,15 @@ const char *configEnumGetName(configEnum *ce, int val) {
Config::Config() {
struct FieldWrapper {
- const char *name;
+ const std::string name;
Review Comment:
Why do you make this change?
--
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 #672: enhance: use unique_ptr in Config
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #672:
URL: https://github.com/apache/incubator-kvrocks/pull/672#discussion_r906783986
##########
src/config.cc:
##########
@@ -84,11 +84,15 @@ const char *configEnumGetName(configEnum *ce, int val) {
Config::Config() {
struct FieldWrapper {
- const char *name;
+ const std::string name;
Review Comment:
```suggestion
std::string name;
```
--
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 #672: enhance: use unique_ptr in Config
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #672:
URL: https://github.com/apache/incubator-kvrocks/pull/672#discussion_r906778885
##########
src/config.cc:
##########
@@ -181,10 +185,10 @@ Config::Config() {
{"rocksdb.level_compaction_dynamic_level_bytes",
false, new YesNoField(&RocksDB.level_compaction_dynamic_level_bytes, false)},
};
- for (const auto &wrapper : fields) {
- auto field = wrapper.field;
+ for (auto &wrapper : fields) {
+ auto &field = wrapper.field;
field->readonly = wrapper.readonly;
- fields_.insert({wrapper.name, field});
+ fields_.insert({wrapper.name, std::move(field)});
Review Comment:
```suggestion
fields_.insert({std::move(wrapper.name), std::move(field)});
```
We can also move the name string 🤣
--
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 #672: enhance: use unique_ptr in Config
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #672:
URL: https://github.com/apache/incubator-kvrocks/pull/672#issuecomment-1166315358
```c++
template<typename T, typename ...Args> auto UniqueConfig(Args &&... args) {
return std::unique_ptr<ConfigField>(new T(std::forward<Args>(args)...));
}
```
You can add this utility function to smplify the changes : )
--
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] jackwener commented on pull request #672: enhance: use unique_ptr in Config
Posted by GitBox <gi...@apache.org>.
jackwener commented on PR #672:
URL: https://github.com/apache/incubator-kvrocks/pull/672#issuecomment-1166438986
Thanks @PragmaTwice ! ❤️👍
--
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 #672: enhance: use unique_ptr in Config
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #672:
URL: https://github.com/apache/incubator-kvrocks/pull/672#discussion_r906817561
##########
src/config.cc:
##########
@@ -84,11 +84,15 @@ const char *configEnumGetName(configEnum *ce, int val) {
Config::Config() {
struct FieldWrapper {
- const char *name;
+ const std::string name;
Review Comment:
`const std::string name` may prevent `std::move` from FieldWrapper::name to key of `std::map field_` in https://github.com/apache/incubator-kvrocks/blob/8836bc57d18927534c928d0b8b7d19a5d93c9d5f/src/config.cc#L191
--
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] jackwener commented on pull request #672: enhance: use unique_ptr in Config
Posted by GitBox <gi...@apache.org>.
jackwener commented on PR #672:
URL: https://github.com/apache/incubator-kvrocks/pull/672#issuecomment-1166405320
> ```c++
> template<typename T, typename ...Args> auto UniqueConfig(Args &&... args) {
> return std::unique_ptr<ConfigField>(new T(std::forward<Args>(args)...));
> }
> ```
>
> You can add this utility function to smplify the changes : )
Looks like use another constructor is clearest.
--
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 #672: enhance: use unique_ptr in Config
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #672:
URL: https://github.com/apache/incubator-kvrocks/pull/672#discussion_r906817561
##########
src/config.cc:
##########
@@ -84,11 +84,15 @@ const char *configEnumGetName(configEnum *ce, int val) {
Config::Config() {
struct FieldWrapper {
- const char *name;
+ const std::string name;
Review Comment:
`const std::string` may prevent `std::move` from FieldWrapper::name to key of `std::map` in https://github.com/apache/incubator-kvrocks/blob/8836bc57d18927534c928d0b8b7d19a5d93c9d5f/src/config.cc#L191
--
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 #672: enhance: use unique_ptr in Config
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #672:
URL: https://github.com/apache/incubator-kvrocks/pull/672#discussion_r906778885
##########
src/config.cc:
##########
@@ -181,10 +185,10 @@ Config::Config() {
{"rocksdb.level_compaction_dynamic_level_bytes",
false, new YesNoField(&RocksDB.level_compaction_dynamic_level_bytes, false)},
};
- for (const auto &wrapper : fields) {
- auto field = wrapper.field;
+ for (auto &wrapper : fields) {
+ auto &field = wrapper.field;
field->readonly = wrapper.readonly;
- fields_.insert({wrapper.name, field});
+ fields_.insert({wrapper.name, std::move(field)});
Review Comment:
```suggestion
fields_.emplace(std::move(wrapper.name), std::move(field));
```
We can also move the name string 🤣
--
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 #672: enhance: use unique_ptr in Config
Posted by GitBox <gi...@apache.org>.
PragmaTwice merged PR #672:
URL: https://github.com/apache/incubator-kvrocks/pull/672
--
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 #672: enhance: use unique_ptr in Config
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #672:
URL: https://github.com/apache/incubator-kvrocks/pull/672#discussion_r906750050
##########
src/config.cc:
##########
@@ -84,11 +84,15 @@ const char *configEnumGetName(configEnum *ce, int val) {
Config::Config() {
struct FieldWrapper {
- const char *name;
+ const std::string name;
bool readonly;
- ConfigField *field;
+ std::unique_ptr<ConfigField> field;
+
+ FieldWrapper(std::string name, bool readonly,
+ ConfigField* field)
+ : name(std::move(name)), readonly(readonly), field(std::move(field)) {}
Review Comment:
Applying `std::move` to a pointer `ConfigField* field` is useless.
--
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