Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor protocol literals into constants #3847

Merged
merged 3 commits into from
Mar 14, 2020

Conversation

JSYoo5B
Copy link
Contributor

@JSYoo5B JSYoo5B commented Mar 14, 2020

Protcol string for PGM, EPGM and NORM is still represented in literal strings.

While changing literals into constants, I also applied conditional compile for them.
Some code blocks didn't covered with conditional compile, and the others is using both PGM and NORM.
I tried to change those code blocks as simple as possible, but some code blocks seems messy.

I want someone review and comment especially for following code blocks. (I've marked WiP just for review)
Code block1
Code block2
Code block3

src/socket_base.cpp Outdated Show resolved Hide resolved
Solution: replace into named constants
Conditinoal compile for OPENPGM and NORM is mixed.
Also found few codes which needs conditional compile but not applied.

Solution: Apply conditional compile preprocessors
@JSYoo5B
Copy link
Contributor Author

JSYoo5B commented Mar 14, 2020

Added new commit to resolve previous ifdef tricks, rebased from master branch.

@JSYoo5B JSYoo5B requested a review from bluca March 14, 2020 12:03
@bluca
Copy link
Member

bluca commented Mar 14, 2020

there's a formatting problem

@JSYoo5B
Copy link
Contributor Author

JSYoo5B commented Mar 14, 2020

Fixed formatting issue, ammended in last commit

@bluca
Copy link
Member

bluca commented Mar 14, 2020

still errors:

--- a/src/socket_base.cpp
+++ b/src/socket_base.cpp
@@ -1038,17 +1038,16 @@ int zmq::socket_base_t::connect_internal (const char *endpoint_uri_)
     //  PGM does not support subscription forwarding; ask for all data to be
     //  sent to this pipe. (same for NORM, currently?)
 #if defined ZMQ_HAVE_OPENPGM && defined ZMQ_HAVE_NORM
-    const bool subscribe_to_all = protocol == protocol_name::pgm
-                                  || protocol == protocol_name::epgm
-                                  || protocol == protocol_name::norm
-                                  || protocol == protocol_name::udp;
+    const bool subscribe_to_all =
+      protocol == protocol_name::pgm || protocol == protocol_name::epgm
+      || protocol == protocol_name::norm || protocol == protocol_name::udp;
 #elif defined ZMQ_HAVE_OPENPGM
     const bool subscribe_to_all = protocol == protocol_name::pgm
                                   || protocol == protocol_name::epgm
                                   || protocol == protocol_name::udp;
 #elif defined ZMQ_HAVE_NORM
-    const bool subscribe_to_all = protocol == protocol_name::norm
-                                  || protocol == protocol_name::udp;
+    const bool subscribe_to_all =
+      protocol == protocol_name::norm || protocol == protocol_name::udp;
 #else
     const bool subscribe_to_all = protocol == protocol_name::udp;
 #endif

Some ifdefs in condition checking may cause problem in some compiler or
static analyzers. When PGM and NORM both are disabled, some condition
will be derived as false || false.

Solution: Splitted condition checking for every ifdef conditions
@JSYoo5B
Copy link
Contributor Author

JSYoo5B commented Mar 14, 2020

I thought only tab->space problems. I just realized using clang-format.

clang-format patches applied.

@bluca bluca merged commit bb9135d into zeromq:master Mar 14, 2020
@JSYoo5B JSYoo5B changed the title [WiP] Refactor protocol literals into constants Refactor protocol literals into constants Mar 14, 2020
@JSYoo5B JSYoo5B deleted the protocol-literals-refactor branch March 14, 2020 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants