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 16:55:07 UTC
[GitHub] [incubator-kvrocks] zz-jason opened a new pull request, #673: use unique_ptr in src/server.h
zz-jason opened a new pull request, #673:
URL: https://github.com/apache/incubator-kvrocks/pull/673
Signed-off-by: Jian Zhang <zj...@gmail.com>
--
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] zz-jason commented on pull request #673: use unique_ptr in src/server.h
Posted by GitBox <gi...@apache.org>.
zz-jason commented on PR #673:
URL: https://github.com/apache/incubator-kvrocks/pull/673#issuecomment-1166411668
I find the help message of `cppcheck` is:
```
--suppress=<spec> Suppress warnings that match <spec>. The format of
<spec> is:
[error id]:[filename]:[line]
The [filename] and [line] are optional. If [error id]
is a wildcard '*', all error ids match.
```
Does `--suppress=noCopyConstructor:src/server.cc` mean to ignore the `noCopyConstructor` warning for `src/server.cc`?
--
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 #673: use unique_ptr in src/server.h
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #673:
URL: https://github.com/apache/incubator-kvrocks/pull/673#discussion_r906747134
##########
src/server.h:
##########
@@ -198,10 +198,10 @@ class Server {
Stats stats_;
Engine::Storage *storage_;
- Cluster *cluster_;
+ std::unique_ptr<Cluster> cluster_;
static std::atomic<int> unix_time_;
- class SlotMigrate *slot_migrate_ = nullptr;
- class SlotImport *slot_import_ = nullptr;
+ std::unique_ptr<class SlotMigrate> slot_migrate_;
+ class SlotImport *slot_import_;
Review Comment:
```suggestion
class SlotImport *slot_import_ = nullptr;
```
fix the cppcheck warning in CI.
--
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 #673: use unique_ptr in src/server.h
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #673:
URL: https://github.com/apache/incubator-kvrocks/pull/673#issuecomment-1166552855
Thanks for @zz-jason 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] PragmaTwice commented on pull request #673: use unique_ptr in src/server.h
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #673:
URL: https://github.com/apache/incubator-kvrocks/pull/673#issuecomment-1166409363
Could you remove `--suppress=noCopyConstructor:src/server.cc --suppress=noOperatorEq:src/server.cc` in `./cppcheck.sh` to make CI pass? Sorry for discommodity.
--
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 pull request #673: use unique_ptr in src/server.h
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #673:
URL: https://github.com/apache/incubator-kvrocks/pull/673#issuecomment-1166352317
This patch can be blocked by #645 as `missingInclude` is tricky to address. See also https://github.com/apache/incubator-kvrocks/pull/645#issuecomment-1162042973.
--
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 #673: use unique_ptr in src/server.h
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #673:
URL: https://github.com/apache/incubator-kvrocks/pull/673#issuecomment-1166412545
> I find the help message of `cppcheck` is:
>
> ```
> --suppress=<spec> Suppress warnings that match <spec>. The format of
> <spec> is:
> [error id]:[filename]:[line]
> The [filename] and [line] are optional. If [error id]
> is a wildcard '*', all error ids match.
> ```
>
> Does `--suppress=noCopyConstructor:src/server.cc` mean to ignore the `noCopyConstructor` warning for `src/server.cc`?
Yeah, my guess is that the `noCopyConstructor` warning no longer appears because you changed the type of some of `Server`'s fields to `unique_ptr` (which has no copy ctor) so that `Server` can no longer copy constructible.
At this point cppcheck reports `unmatchedSuppression` because we added an useless `-suppress`.
We will consider using clang tidy instead of cppcheck in the future.
```
src/server.cc:-1:0: information: Unmatched suppression: noCopyConstructor [unmatchedSuppression]
^
src/server.cc:-1:0: information: Unmatched suppression: noOperatorEq [unmatchedSuppression]
^
```
Actually you can add `--suppress=unmatchedSuppression` instead of remove anything.
--
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 #673: use unique_ptr in src/server.h
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #673:
URL: https://github.com/apache/incubator-kvrocks/pull/673#issuecomment-1166482051
> LGTM.
>
> @git-hulk @PragmaTwice it seems we don't have `delete cluster_` to eliminate in this patch, did we leak memory previously on these field?
I also wonder that. 🤣
--
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 #673: use unique_ptr in src/server.h
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #673:
URL: https://github.com/apache/incubator-kvrocks/pull/673#issuecomment-1166519444
@tisonkun @PragmaTwice Yes, we do leak this field even it wouldn't cause problem.
--
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 #673: use unique_ptr in src/server.h
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #673:
URL: https://github.com/apache/incubator-kvrocks/pull/673#discussion_r906747134
##########
src/server.h:
##########
@@ -198,10 +198,10 @@ class Server {
Stats stats_;
Engine::Storage *storage_;
- Cluster *cluster_;
+ std::unique_ptr<Cluster> cluster_;
static std::atomic<int> unix_time_;
- class SlotMigrate *slot_migrate_ = nullptr;
- class SlotImport *slot_import_ = nullptr;
+ std::unique_ptr<class SlotMigrate> slot_migrate_;
+ class SlotImport *slot_import_;
Review Comment:
```suggestion
class SlotImport *slot_import_ = nullptr;
```
fix the cppcheck warning in CI
--
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 merged pull request #673: use unique_ptr in src/server.h
Posted by GitBox <gi...@apache.org>.
git-hulk merged PR #673:
URL: https://github.com/apache/incubator-kvrocks/pull/673
--
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