You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pegasus.apache.org by GitBox <gi...@apache.org> on 2022/11/19 02:32:49 UTC
[GitHub] [incubator-pegasus] acelyc111 opened a new pull request, #1248: refactor: enable warning on unused result
acelyc111 opened a new pull request, #1248:
URL: https://github.com/apache/incubator-pegasus/pull/1248
https://github.com/apache/incubator-pegasus/issues/1166
Deal with function return value to ensure seriours error could be found immediatelly.
You can add macro `WARN_UNUSED_RESULT` after the function declaration to mark the function's return value must be dealt.
This patch add the macro and make `command_manager::register_command` as `WARN_UNUSED_RESULT`, all callers have been refactored to adapt this semantics.
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1248: refactor: enable warning on unused result
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1248:
URL: https://github.com/apache/incubator-pegasus/pull/1248#discussion_r1029405866
##########
src/block_service/directio_writable_file.cpp:
##########
@@ -96,12 +103,16 @@ bool direct_io_writable_file::finalize()
if (_offset > 0) {
if (::write(_fd, _buffer, _buffer_size) != _buffer_size) {
Review Comment:
We can do that, but not in this patch.
TODO added.
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] empiredan commented on a diff in pull request #1248: refactor: enable warning on unused result
Posted by GitBox <gi...@apache.org>.
empiredan commented on code in PR #1248:
URL: https://github.com/apache/incubator-pegasus/pull/1248#discussion_r1028249160
##########
src/block_service/test/fds_service_test.cpp:
##########
@@ -634,8 +635,8 @@ TEST_F(FDSClientTest, test_basic_operation)
static void
generate_file(const char *filename, unsigned long long file_size, char *block, unsigned block_size)
{
- int fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP);
- ASSERT_TRUE(fd > 0) << strerror(errno) << std::endl;
+ int fd = ::open(filename, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP);
+ ASSERT_TRUE(fd > 0) << utils::safe_strerror(errno);
Review Comment:
```suggestion
ASSERT_GT(fd, 0) << utils::safe_strerror(errno);
```
##########
src/block_service/directio_writable_file.cpp:
##########
@@ -96,12 +103,16 @@ bool direct_io_writable_file::finalize()
if (_offset > 0) {
if (::write(_fd, _buffer, _buffer_size) != _buffer_size) {
Review Comment:
Once returned value by `::write()` is non-negative, which means some bytes might have been written, should we retry remaining bytes several times until all data is written into the file ?
##########
src/block_service/test/fds_service_test.cpp:
##########
@@ -645,9 +646,11 @@ generate_file(const char *filename, unsigned long long file_size, char *block, u
for (int j = 0; j < batch_size; ++j) {
block[j] = (char)rand::next_u32(0, 255);
}
- write(fd, block, batch_size);
+ ASSERT_EQ(batch_size, ::write(fd, block, batch_size))
+ << "write file " << filename << " failed, err = {}" << utils::safe_strerror(errno);
Review Comment:
```suggestion
<< "write file " << filename << " failed, err = " << utils::safe_strerror(errno);
```
##########
src/utils/logging.cpp:
##########
@@ -158,13 +128,11 @@ void dsn_log(const char *file,
namespace dsn {
-std::unique_ptr<logging_provider> logging_provider::_logger =
- std::unique_ptr<logging_provider>(nullptr);
+std::unique_ptr<logging_provider> logging_provider::_logger = nullptr;
Review Comment:
```suggestion
std::unique_ptr<logging_provider> logging_provider::_logger;
```
##########
src/utils/logging_provider.h:
##########
@@ -80,10 +74,16 @@ class logging_provider
virtual void flush() = 0;
-private:
+ void deregister_commands() { _cmds.clear(); }
+
+protected:
+ logging_provider(const char *) {}
Review Comment:
```suggestion
logging_provider(const char *) = default;
```
##########
src/utils/simple_logger.h:
##########
@@ -71,22 +72,22 @@ class simple_logger : public logging_provider
{
public:
simple_logger(const char *log_dir);
- virtual ~simple_logger(void);
+ ~simple_logger(void);
Review Comment:
Should we mark `override` for those destructors whose ancestors are declared as `virtual` ?
##########
src/block_service/test/fds_service_test.cpp:
##########
@@ -645,9 +646,11 @@ generate_file(const char *filename, unsigned long long file_size, char *block, u
for (int j = 0; j < batch_size; ++j) {
block[j] = (char)rand::next_u32(0, 255);
}
- write(fd, block, batch_size);
+ ASSERT_EQ(batch_size, ::write(fd, block, batch_size))
+ << "write file " << filename << " failed, err = {}" << utils::safe_strerror(errno);
}
- close(fd);
+ ASSERT_EQ(0, ::close(fd)) << "close file " << filename << " failed, err = {}"
Review Comment:
```suggestion
ASSERT_EQ(0, ::close(fd)) << "close file " << filename << " failed, err = "
```
##########
src/block_service/directio_writable_file.cpp:
##########
@@ -96,12 +103,16 @@ bool direct_io_writable_file::finalize()
if (_offset > 0) {
if (::write(_fd, _buffer, _buffer_size) != _buffer_size) {
- LOG_ERROR_F(
- "Failed to write last chunk, filie_path = {}, errno = {}", _file_path, errno);
+ LOG_ERROR_F("Failed to write last chunk, file_path = {}, err = {}",
+ _file_path,
+ utils::safe_strerror(errno));
Review Comment:
Once returned value by `::write()` is non-negative, `errno` may not be set ?
##########
src/utils/logging_provider.h:
##########
@@ -55,9 +51,7 @@ class logging_provider
typedef logging_provider *(*factory)(const char *);
public:
- logging_provider(const char *) {}
-
- virtual ~logging_provider(void) {}
+ virtual ~logging_provider() {}
Review Comment:
```suggestion
virtual ~logging_provider() = default;
```
##########
src/utils/command_manager.cpp:
##########
@@ -177,10 +178,9 @@ command_manager::command_manager()
command_manager::~command_manager()
{
_cmds.clear();
- _handlers.clear();
- // TODO(yingchun): enable this check when all commands deregister correctly.
- // CHECK(_handlers.empty(), "All commands must be deregistered before command_manager been
- // destroyed", _handlers.begin()->first);
+ CHECK(_handlers.empty(),
+ "All commands must be deregistered before command_manager been destroyed",
Review Comment:
```suggestion
"All commands must be deregistered before command_manager is destroyed, however {} is still registered",
```
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1248: refactor: enable warning on unused result
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1248:
URL: https://github.com/apache/incubator-pegasus/pull/1248#discussion_r1029481494
##########
src/utils/simple_logger.h:
##########
@@ -71,22 +72,22 @@ class simple_logger : public logging_provider
{
public:
simple_logger(const char *log_dir);
- virtual ~simple_logger(void);
+ ~simple_logger(void);
Review Comment:
I think it would be better to mark `override`.
https://github.com/isocpp/CppCoreGuidelines/issues/721
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1248: refactor: enable warning on unused result
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1248:
URL: https://github.com/apache/incubator-pegasus/pull/1248#discussion_r1029508881
##########
src/utils/logging_provider.h:
##########
@@ -80,10 +74,16 @@ class logging_provider
virtual void flush() = 0;
-private:
+ void deregister_commands() { _cmds.clear(); }
+
+protected:
+ logging_provider(const char *) {}
Review Comment:
Removed it, we don't 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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1248: refactor: enable warning on unused result
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1248:
URL: https://github.com/apache/incubator-pegasus/pull/1248#discussion_r1029405866
##########
src/block_service/directio_writable_file.cpp:
##########
@@ -96,12 +103,16 @@ bool direct_io_writable_file::finalize()
if (_offset > 0) {
if (::write(_fd, _buffer, _buffer_size) != _buffer_size) {
Review Comment:
Updated to distinguish the different errors.
We can do that in another patch, TODO added.
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] acelyc111 merged pull request #1248: refactor: enable warning on unused result
Posted by GitBox <gi...@apache.org>.
acelyc111 merged PR #1248:
URL: https://github.com/apache/incubator-pegasus/pull/1248
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org