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/08/17 17:55:37 UTC
[GitHub] [incubator-kvrocks] Furud0Er1ka opened a new pull request, #781: replace new instead of std::make_unique or std::make_shared
Furud0Er1ka opened a new pull request, #781:
URL: https://github.com/apache/incubator-kvrocks/pull/781
1. replaced new instead of std::make_unique or std::make_shared
2. replace some NULLs instead of nullptr
3. replace condition == false instead of !condition
--
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 #781: replace new instead of std::make_unique or std::make_shared
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #781:
URL: https://github.com/apache/incubator-kvrocks/pull/781#discussion_r949365877
##########
src/util.h:
##########
@@ -71,8 +76,14 @@ uint64_t GetTimeStampUS(void);
// define std::make_unique in c++14
// refer to https://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique
template <typename T, typename... Args>
-std::unique_ptr<T> MakeUnique(Args&& ... args) {
+inline std::unique_ptr<T> MakeUnique(Args &&...args) {
Review Comment:
This `inline` is useless. A function template does already imply an `inline`.
--
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 #781: replace new instead of std::make_unique or std::make_shared
Posted by GitBox <gi...@apache.org>.
PragmaTwice merged PR #781:
URL: https://github.com/apache/incubator-kvrocks/pull/781
--
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] Furud0Er1ka commented on pull request #781: replace new instead of std::make_unique or std::make_shared
Posted by GitBox <gi...@apache.org>.
Furud0Er1ka commented on PR #781:
URL: https://github.com/apache/incubator-kvrocks/pull/781#issuecomment-1219717161
> Hi, as mentioned by @git-hulk, I think you could minimize your PR, and you can check here to see why cpplint failed: https://github.com/apache/incubator-kvrocks/runs/7902217149?check_suite_focus=true
thanks for your review! I have removed all unnecessary 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] git-hulk commented on pull request #781: replace new instead of std::make_unique or std::make_shared
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #781:
URL: https://github.com/apache/incubator-kvrocks/pull/781#issuecomment-1220163871
cool, thanks
--
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 #781: replace new instead of std::make_unique or std::make_shared
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #781:
URL: https://github.com/apache/incubator-kvrocks/pull/781#issuecomment-1221575396
Thanks for your contribution again! 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] PragmaTwice commented on a diff in pull request #781: replace new instead of std::make_unique or std::make_shared
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #781:
URL: https://github.com/apache/incubator-kvrocks/pull/781#discussion_r949822088
##########
src/util.h:
##########
@@ -75,4 +75,8 @@ std::unique_ptr<T> MakeUnique(Args&& ... args) {
return std::unique_ptr<T>(new T(std::forward<Args>(args)...));
}
+template <typename T, typename... Args>
+std::shared_ptr<T> MakeShared(Args&& ... args) {
+ return std::shared_ptr<T>(new T(std::forward<Args>(args)...));
Review Comment:
```suggestion
return std::make_shared<T>(std::forward<Args>(args)...);
```
--
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 pull request #781: replace new instead of std::make_unique or std::make_shared
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #781:
URL: https://github.com/apache/incubator-kvrocks/pull/781#issuecomment-1219662776
Hello @Furud0Er1ka, I saw many places are only formatted without changing the logic, I think we can rollback those format codes to keep this PR consistent with what's title described, it also helps the code review easier.
--
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 #781: replace new instead of std::make_unique or std::make_shared
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #781:
URL: https://github.com/apache/incubator-kvrocks/pull/781#discussion_r949739157
##########
src/util.h:
##########
@@ -74,5 +74,8 @@ template <typename T, typename... Args>
std::unique_ptr<T> MakeUnique(Args&& ... args) {
return std::unique_ptr<T>(new T(std::forward<Args>(args)...));
}
-
+template <typename T, typename... Args>
Review Comment:
```suggestion
template <typename T, typename... Args>
```
--
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 #781: replace new instead of std::make_unique or std::make_shared
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #781:
URL: https://github.com/apache/incubator-kvrocks/pull/781#discussion_r949364315
##########
src/util.h:
##########
@@ -71,8 +76,14 @@ uint64_t GetTimeStampUS(void);
// define std::make_unique in c++14
// refer to https://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique
template <typename T, typename... Args>
-std::unique_ptr<T> MakeUnique(Args&& ... args) {
+inline std::unique_ptr<T> MakeUnique(Args &&...args) {
return std::unique_ptr<T>(new T(std::forward<Args>(args)...));
}
-} // namespace Util
+// define std::make_shared in c++14
+// refer to https://en.cppreference.com/w/cpp/memory/shared_ptr/make_shared
+template <typename T, typename... Args>
+inline std::shared_ptr<T> MakeShared(Args &&...args) {
+ return std::shared_ptr<T>(new T(std::forward<Args>(args)...));
+}
+} // namespace Util
Review Comment:
`std::make_shared` is in c++11, not c++14, so I think we do not need it.
--
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 #781: replace new instead of std::make_unique or std::make_shared
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #781:
URL: https://github.com/apache/incubator-kvrocks/pull/781#issuecomment-1219705138
Hi, as mentioned by @git-hulk, I think you could minimize your PR, and you can check here to see why cpplint failed: https://github.com/apache/incubator-kvrocks/runs/7902217149?check_suite_focus=true
--
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 #781: replace new instead of std::make_unique or std::make_shared
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #781:
URL: https://github.com/apache/incubator-kvrocks/pull/781#discussion_r949365877
##########
src/util.h:
##########
@@ -71,8 +76,14 @@ uint64_t GetTimeStampUS(void);
// define std::make_unique in c++14
// refer to https://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique
template <typename T, typename... Args>
-std::unique_ptr<T> MakeUnique(Args&& ... args) {
+inline std::unique_ptr<T> MakeUnique(Args &&...args) {
Review Comment:
This `inline` is useless. A function template does already imply `inline`.
--
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 #781: replace new instead of std::make_unique or std::make_shared
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #781:
URL: https://github.com/apache/incubator-kvrocks/pull/781#issuecomment-1219058556
Hi, thanks for your contribution.
Since there is no `std::make_unique` in c++11, you can use `Util::MakeUnique` in `util.h` as an alternative.
--
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