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/10/13 04:22:33 UTC
[GitHub] [incubator-kvrocks] PragmaTwice opened a new pull request, #981: Replace constant macros to constexpr variables and enums in headers
PragmaTwice opened a new pull request, #981:
URL: https://github.com/apache/incubator-kvrocks/pull/981
Since macros have some defect because only the text replacement is performed, I think we can encourage developers to use more constexpr variables, function templates and enums while it is possible.
--
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 #981: Replace constant macros to constexpr variables and enums in headers
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #981:
URL: https://github.com/apache/incubator-kvrocks/pull/981#discussion_r994175518
##########
src/config.h:
##########
@@ -38,20 +38,18 @@ class Server;
namespace Engine {
class Storage;
}
-#define PORT_LIMIT 65535
-#define SUPERVISED_NONE 0
-#define SUPERVISED_AUTODETECT 1
-#define SUPERVISED_SYSTEMD 2
-#define SUPERVISED_UPSTART 3
+constexpr const uint16_t PORT_LIMIT = 65535;
-#define TLS_AUTH_CLIENTS_NO "no"
-#define TLS_AUTH_CLIENTS_OPTIONAL "optional"
+enum { SUPERVISED_NONE = 0, SUPERVISED_AUTODETECT, SUPERVISED_SYSTEMD, SUPERVISED_UPSTART };
Review Comment:
Would you like to give a name to this enum?
##########
src/redis_zset.h:
##########
@@ -76,11 +76,13 @@ typedef struct {
double score;
} MemberScore;
-#define ZSET_INCR 1
-#define ZSET_NX (1 << 1)
-#define ZSET_XX (1 << 2)
-#define ZSET_REVERSED (1 << 3)
-#define ZSET_REMOVED 1 << 4
+enum {
Review Comment:
And to give a name for `enum` here to add some extra meaning to these values?
--
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 #981: Replace constant macros to constexpr variables and enums in headers
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #981:
URL: https://github.com/apache/incubator-kvrocks/pull/981#discussion_r994433247
##########
src/redis_zset.h:
##########
@@ -76,11 +76,13 @@ typedef struct {
double score;
} MemberScore;
-#define ZSET_INCR 1
-#define ZSET_NX (1 << 1)
-#define ZSET_XX (1 << 2)
-#define ZSET_REVERSED (1 << 3)
-#define ZSET_REMOVED 1 << 4
+enum ZSetFlags {
Review Comment:
What do you think about the naming of enumerating options? Because they aren't macro definitions now, maybe they could have naming like `MigrateTaskState` (camel case with the `k` prefix as described here https://google.github.io/styleguide/cppguide.html#Enumerator_Names)?
--
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 #981: Replace constant macros to constexpr variables and enums in headers
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #981:
URL: https://github.com/apache/incubator-kvrocks/pull/981#issuecomment-1277215078
> Hi. Is it better to use `constexpr` here too? https://github.com/apache/incubator-kvrocks/blob/unstable/src/redis_cmd.cc#L63
Sure. I think at least it should be typed `const char * const`, since the top-level `const` qualifier matters for constants (in linkage and storage durations).
--
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 #981: Replace constant macros to constexpr variables and enums in headers
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #981:
URL: https://github.com/apache/incubator-kvrocks/pull/981#discussion_r994306583
##########
src/config.h:
##########
@@ -38,20 +38,18 @@ class Server;
namespace Engine {
class Storage;
}
-#define PORT_LIMIT 65535
-#define SUPERVISED_NONE 0
-#define SUPERVISED_AUTODETECT 1
-#define SUPERVISED_SYSTEMD 2
-#define SUPERVISED_UPSTART 3
+constexpr const uint16_t PORT_LIMIT = 65535;
-#define TLS_AUTH_CLIENTS_NO "no"
-#define TLS_AUTH_CLIENTS_OPTIONAL "optional"
+enum { SUPERVISED_NONE = 0, SUPERVISED_AUTODETECT, SUPERVISED_SYSTEMD, SUPERVISED_UPSTART };
Review Comment:
done
##########
src/redis_zset.h:
##########
@@ -76,11 +76,13 @@ typedef struct {
double score;
} MemberScore;
-#define ZSET_INCR 1
-#define ZSET_NX (1 << 1)
-#define ZSET_XX (1 << 2)
-#define ZSET_REVERSED (1 << 3)
-#define ZSET_REMOVED 1 << 4
+enum {
Review Comment:
done
--
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 #981: Replace constant macros to constexpr variables and enums in headers
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #981:
URL: https://github.com/apache/incubator-kvrocks/pull/981#discussion_r994314170
##########
src/config.h:
##########
@@ -38,20 +38,18 @@ class Server;
namespace Engine {
class Storage;
}
-#define PORT_LIMIT 65535
-#define SUPERVISED_NONE 0
-#define SUPERVISED_AUTODETECT 1
-#define SUPERVISED_SYSTEMD 2
-#define SUPERVISED_UPSTART 3
+constexpr const uint16_t PORT_LIMIT = 65535;
-#define TLS_AUTH_CLIENTS_NO "no"
-#define TLS_AUTH_CLIENTS_OPTIONAL "optional"
+enum SupervisedMode { SUPERVISED_NONE = 0, SUPERVISED_AUTODETECT, SUPERVISED_SYSTEMD, SUPERVISED_UPSTART };
Review Comment:
I think we cannot remove it. Since it is not an `enum class`, the names in the `enum` will be directly exported to the outer namespace, so if we remove the prefix, we will get some name like `NONE` in the gloabl namespace, which will be a mess.
--
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] caipengbo commented on a diff in pull request #981: Replace constant macros to constexpr variables and enums in headers
Posted by GitBox <gi...@apache.org>.
caipengbo commented on code in PR #981:
URL: https://github.com/apache/incubator-kvrocks/pull/981#discussion_r994319188
##########
src/config.h:
##########
@@ -38,20 +38,18 @@ class Server;
namespace Engine {
class Storage;
}
-#define PORT_LIMIT 65535
-#define SUPERVISED_NONE 0
-#define SUPERVISED_AUTODETECT 1
-#define SUPERVISED_SYSTEMD 2
-#define SUPERVISED_UPSTART 3
+constexpr const uint16_t PORT_LIMIT = 65535;
-#define TLS_AUTH_CLIENTS_NO "no"
-#define TLS_AUTH_CLIENTS_OPTIONAL "optional"
+enum SupervisedMode { SUPERVISED_NONE = 0, SUPERVISED_AUTODETECT, SUPERVISED_SYSTEMD, SUPERVISED_UPSTART };
Review Comment:
Ok, got 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 merged pull request #981: Replace constant macros to constexpr variables and enums in headers
Posted by GitBox <gi...@apache.org>.
PragmaTwice merged PR #981:
URL: https://github.com/apache/incubator-kvrocks/pull/981
--
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] tanruixiang commented on pull request #981: Replace constant macros to constexpr variables and enums in headers
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #981:
URL: https://github.com/apache/incubator-kvrocks/pull/981#issuecomment-1277027541
Hi. Is it better to use `constexpr` here too?
https://github.com/apache/incubator-kvrocks/blob/unstable/src/redis_cmd.cc#L63
--
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 #981: Replace constant macros to constexpr variables and enums in headers
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #981:
URL: https://github.com/apache/incubator-kvrocks/pull/981#discussion_r994314170
##########
src/config.h:
##########
@@ -38,20 +38,18 @@ class Server;
namespace Engine {
class Storage;
}
-#define PORT_LIMIT 65535
-#define SUPERVISED_NONE 0
-#define SUPERVISED_AUTODETECT 1
-#define SUPERVISED_SYSTEMD 2
-#define SUPERVISED_UPSTART 3
+constexpr const uint16_t PORT_LIMIT = 65535;
-#define TLS_AUTH_CLIENTS_NO "no"
-#define TLS_AUTH_CLIENTS_OPTIONAL "optional"
+enum SupervisedMode { SUPERVISED_NONE = 0, SUPERVISED_AUTODETECT, SUPERVISED_SYSTEMD, SUPERVISED_UPSTART };
Review Comment:
I think we cannot remove it. Since it is not an `enum class`, the names in the `enum` will be directly exported to the outer namespace, so if we remove the prefix, we will get some name like `NONE` in the gloabl namespace.
--
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 #981: Replace constant macros to constexpr variables and enums in headers
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #981:
URL: https://github.com/apache/incubator-kvrocks/pull/981#discussion_r994451261
##########
src/redis_zset.h:
##########
@@ -76,11 +76,13 @@ typedef struct {
double score;
} MemberScore;
-#define ZSET_INCR 1
-#define ZSET_NX (1 << 1)
-#define ZSET_XX (1 << 2)
-#define ZSET_REVERSED (1 << 3)
-#define ZSET_REMOVED 1 << 4
+enum ZSetFlags {
Review Comment:
done
--
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 #981: Replace constant macros to constexpr variables and enums in headers
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #981:
URL: https://github.com/apache/incubator-kvrocks/pull/981#issuecomment-1277525284
Thanks all. 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 #981: Replace constant macros to constexpr variables and enums in headers
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #981:
URL: https://github.com/apache/incubator-kvrocks/pull/981#discussion_r994314170
##########
src/config.h:
##########
@@ -38,20 +38,18 @@ class Server;
namespace Engine {
class Storage;
}
-#define PORT_LIMIT 65535
-#define SUPERVISED_NONE 0
-#define SUPERVISED_AUTODETECT 1
-#define SUPERVISED_SYSTEMD 2
-#define SUPERVISED_UPSTART 3
+constexpr const uint16_t PORT_LIMIT = 65535;
-#define TLS_AUTH_CLIENTS_NO "no"
-#define TLS_AUTH_CLIENTS_OPTIONAL "optional"
+enum SupervisedMode { SUPERVISED_NONE = 0, SUPERVISED_AUTODETECT, SUPERVISED_SYSTEMD, SUPERVISED_UPSTART };
Review Comment:
I think we cannot remove it. Since it is not an `enum class`, the names in the `enum` will be directly exported to the outer namespace, so if we remove the prefix, we will get some name like `NONE` in the global namespace, which will be a mess.
--
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] caipengbo commented on a diff in pull request #981: Replace constant macros to constexpr variables and enums in headers
Posted by GitBox <gi...@apache.org>.
caipengbo commented on code in PR #981:
URL: https://github.com/apache/incubator-kvrocks/pull/981#discussion_r994311547
##########
src/config.h:
##########
@@ -38,20 +38,18 @@ class Server;
namespace Engine {
class Storage;
}
-#define PORT_LIMIT 65535
-#define SUPERVISED_NONE 0
-#define SUPERVISED_AUTODETECT 1
-#define SUPERVISED_SYSTEMD 2
-#define SUPERVISED_UPSTART 3
+constexpr const uint16_t PORT_LIMIT = 65535;
-#define TLS_AUTH_CLIENTS_NO "no"
-#define TLS_AUTH_CLIENTS_OPTIONAL "optional"
+enum SupervisedMode { SUPERVISED_NONE = 0, SUPERVISED_AUTODETECT, SUPERVISED_SYSTEMD, SUPERVISED_UPSTART };
Review Comment:
Now that enum has a name, can the prefix of enum values be removed?
##########
src/redis_zset.h:
##########
@@ -76,11 +76,13 @@ typedef struct {
double score;
} MemberScore;
-#define ZSET_INCR 1
-#define ZSET_NX (1 << 1)
-#define ZSET_XX (1 << 2)
-#define ZSET_REVERSED (1 << 3)
-#define ZSET_REMOVED 1 << 4
+enum ZSetFlags {
+ ZSET_INCR = 1,
+ ZSET_NX = 1 << 1,
+ ZSET_XX = 1 << 2,
+ ZSET_REVERSED = 1 << 3,
+ ZSET_REMOVED = 1 << 4,
+};
Review Comment:
Can `ZSET_` be removed?
--
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 #981: Replace constant macros to constexpr variables and enums in headers
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #981:
URL: https://github.com/apache/incubator-kvrocks/pull/981#discussion_r994316653
##########
src/config.h:
##########
@@ -38,20 +38,18 @@ class Server;
namespace Engine {
class Storage;
}
-#define PORT_LIMIT 65535
-#define SUPERVISED_NONE 0
-#define SUPERVISED_AUTODETECT 1
-#define SUPERVISED_SYSTEMD 2
-#define SUPERVISED_UPSTART 3
+constexpr const uint16_t PORT_LIMIT = 65535;
-#define TLS_AUTH_CLIENTS_NO "no"
-#define TLS_AUTH_CLIENTS_OPTIONAL "optional"
+enum SupervisedMode { SUPERVISED_NONE = 0, SUPERVISED_AUTODETECT, SUPERVISED_SYSTEMD, SUPERVISED_UPSTART };
Review Comment:
And the reason I do not use `enum class` here is that `enum class` cannot be implicitly casted to integer, so it will involve more code 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