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.

Sign up for GitHub

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

Jump to bottom

GEODE-10096: Replaced macro definitions with enumerations. #938

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

GEODE-10096: Replaced macro definitions with enumerations. #938

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c1d76e4
GEODE-10096: Replaced macro definitions with enumerations.
Mar 2, 2022
35e402f
GEODE-10096: Moved some types to source file, cleaned switch.
Mar 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 35 additions & 17 deletions cppcache/src/TcrConnection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,23 @@
#include "ThinClientRegion.hpp"
#include "Utils.hpp"
#include "Version.hpp"
#include "util/to_underlying.hpp"

#define DEFAULT_TIMEOUT_RETRIES 12

namespace {
enum class Acceptor : ::std::int8_t {
CLIENT_TO_SERVER = 100,
PRIMARY_SERVER_TO_CLIENT = 101,
SECONDARY_SERVER_TO_CLIENT = 102
};

enum class Security : ::std::uint8_t {
SECURITY_CREDENTIALS_NONE = 0,
SECURITY_CREDENTIALS_NORMAL = 1,
SECURITY_MULTIUSER_NOTIFICATIONCHANNEL = 3
};
} // namespace

#define throwException(ex) \
do { \
Expand Down Expand Up @@ -154,18 +171,18 @@ bool TcrConnection::initTcrConnection(
if (isClientNotification) {
isNotificationChannel = true;
if (isSecondary) {
handShakeMsg.write(static_cast<int8_t>(SECONDARY_SERVER_TO_CLIENT));
handShakeMsg.write(to_underlying(Acceptor::SECONDARY_SERVER_TO_CLIENT));
} else {
handShakeMsg.write(static_cast<int8_t>(PRIMARY_SERVER_TO_CLIENT));
handShakeMsg.write(to_underlying(Acceptor::PRIMARY_SERVER_TO_CLIENT));
}
} else {
handShakeMsg.write(static_cast<int8_t>(CLIENT_TO_SERVER));
handShakeMsg.write(to_underlying(Acceptor::CLIENT_TO_SERVER));
}

Version::write(handShakeMsg, Version::current());
LOGFINE("Client version ordinal is %d", Version::current().getOrdinal());

handShakeMsg.write(static_cast<int8_t>(REPLY_OK));
handShakeMsg.write(to_underlying(AcceptanceCode::REPLY_OK));

// Send byte REPLY_OK = (byte)58;
if (!isClientNotification) {
Expand Down Expand Up @@ -241,11 +258,11 @@ bool TcrConnection::initTcrConnection(

if (isNotificationChannel && !doIneedToSendCreds) {
handShakeMsg.write(
static_cast<uint8_t>(SECURITY_MULTIUSER_NOTIFICATIONCHANNEL));
to_underlying(Security::SECURITY_MULTIUSER_NOTIFICATIONCHANNEL));
} else if (tmpIsSecurityOn) {
handShakeMsg.write(static_cast<uint8_t>(SECURITY_CREDENTIALS_NORMAL));
handShakeMsg.write(to_underlying(Security::SECURITY_CREDENTIALS_NORMAL));
} else {
handShakeMsg.write(static_cast<uint8_t>(SECURITY_CREDENTIALS_NONE));
handShakeMsg.write(to_underlying(Security::SECURITY_CREDENTIALS_NONE));
}

if (tmpIsSecurityOn) {
Expand Down Expand Up @@ -304,7 +321,8 @@ bool TcrConnection::initTcrConnection(

LOGDEBUG(" Handshake: Got Accept Code %d", acceptanceCode[0]);
/* adongre */
if (acceptanceCode[0] == REPLY_SSL_ENABLED && !sysProp.sslEnabled()) {
if (acceptanceCode[0] == to_underlying(AcceptanceCode::REPLY_SSL_ENABLED) &&
!sysProp.sslEnabled()) {
LOGERROR("SSL is enabled on server, enable SSL in client as well");
AuthenticationRequiredException ex(
"SSL is enabled on server, enable SSL in client as well");
Expand Down Expand Up @@ -379,34 +397,34 @@ bool TcrConnection::initTcrConnection(
ThinClientBaseDM::setDeltaEnabledOnServer(di.readBoolean());
}

switch (acceptanceCode[0]) {
case REPLY_OK:
case SUCCESSFUL_SERVER_TO_CLIENT:
switch (static_cast<AcceptanceCode>(acceptanceCode[0])) {
case AcceptanceCode::REPLY_OK:
case AcceptanceCode::SUCCESSFUL_SERVER_TO_CLIENT:
LOGFINER("Handshake reply: %u,%u,%u", acceptanceCode[0],
serverQueueStatus[0], recvMsgLen2);
if (isClientNotification) readHandshakeInstantiatorMsg(connectTimeout);
break;
case REPLY_AUTHENTICATION_FAILED: {
case AcceptanceCode::REPLY_AUTHENTICATION_FAILED: {
AuthenticationFailedException ex(
reinterpret_cast<char*>(recvMessage.data()));
m_conn.reset();
throwException(ex);
}
case REPLY_AUTHENTICATION_REQUIRED: {
case AcceptanceCode::REPLY_AUTHENTICATION_REQUIRED: {
AuthenticationRequiredException ex(
reinterpret_cast<char*>(recvMessage.data()));
m_conn.reset();
throwException(ex);
}
case REPLY_DUPLICATE_DURABLE_CLIENT: {
case AcceptanceCode::REPLY_DUPLICATE_DURABLE_CLIENT: {
DuplicateDurableClientException ex(
reinterpret_cast<char*>(recvMessage.data()));
m_conn.reset();
throwException(ex);
}
case REPLY_REFUSED:
case REPLY_INVALID:
case UNSUCCESSFUL_SERVER_TO_CLIENT: {
case AcceptanceCode::REPLY_REFUSED:
case AcceptanceCode::REPLY_INVALID:
case AcceptanceCode::UNSUCCESSFUL_SERVER_TO_CLIENT: {
LOGERROR("Handshake rejected by server[%s]: %s",
m_endpointObj->name().c_str(),
reinterpret_cast<char*>(recvMessage.data()));
Expand Down
28 changes: 11 additions & 17 deletions cppcache/src/TcrConnection.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,23 +32,17 @@
#include "util/concurrent/binary_semaphore.hpp"
#include "util/synchronized_set.hpp"

#define DEFAULT_TIMEOUT_RETRIES 12
#define PRIMARY_SERVER_TO_CLIENT 101
#define SECONDARY_SERVER_TO_CLIENT 102
#define SUCCESSFUL_SERVER_TO_CLIENT 105
#define UNSUCCESSFUL_SERVER_TO_CLIENT 106
#define CLIENT_TO_SERVER 100
#define REPLY_OK 59
#define REPLY_REFUSED 60
#define REPLY_INVALID 61
#define REPLY_SSL_ENABLED 21
#define REPLY_AUTHENTICATION_REQUIRED 62
#define REPLY_AUTHENTICATION_FAILED 63
#define REPLY_DUPLICATE_DURABLE_CLIENT 64

#define SECURITY_CREDENTIALS_NONE 0
#define SECURITY_CREDENTIALS_NORMAL 1
#define SECURITY_MULTIUSER_NOTIFICATIONCHANNEL 3
enum class AcceptanceCode : ::std::int8_t {
REPLY_SSL_ENABLED = 21,
REPLY_OK = 59,
REPLY_REFUSED = 60,
REPLY_INVALID = 61,
REPLY_AUTHENTICATION_REQUIRED = 62,
REPLY_AUTHENTICATION_FAILED = 63,
REPLY_DUPLICATE_DURABLE_CLIENT = 64,
SUCCESSFUL_SERVER_TO_CLIENT = 105,
UNSUCCESSFUL_SERVER_TO_CLIENT = 106
};

namespace apache {
namespace geode {
Expand Down
4 changes: 3 additions & 1 deletion cppcache/src/ThinClientLocatorHelper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
#include "TcrConnectionManager.hpp"
#include "ThinClientPoolDM.hpp"
#include "Version.hpp"
#include "util/to_underlying.hpp"

INIT_GNFN("ThinClientLocatorHelper")

Expand Down Expand Up @@ -158,7 +159,8 @@ std::shared_ptr<Serializable> ThinClientLocatorHelper::sendRequest(
auto di = m_poolDM->getConnectionManager().getCacheImpl()->createDataInput(
reinterpret_cast<uint8_t*>(buff), receivedLength);

if (di.read() == REPLY_SSL_ENABLED && !sys_prop.sslEnabled()) {
if (di.read() == to_underlying(AcceptanceCode::REPLY_SSL_ENABLED) &&
!sys_prop.sslEnabled()) {
LOGERROR(
"%s(%p): SSL is enabled on locator %s, enable SSL in client as well",
__GNFN__, this, location.toString().c_str());
Expand Down
35 changes: 35 additions & 0 deletions cppcache/src/util/to_underlying.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#ifndef UTIL_UNDERLYING_TYPE_HPP
#define UTIL_UNDERLYING_TYPE_HPP

#include <type_traits>

/* Note: this implementation can be thrown away once we upgrade to C++14. */

template< class T >
using underlying_type_t = typename std::underlying_type<T>::type;

/* Note: this implementation can be thrown away once we upgrade to C++23. */

template< class Enum >
constexpr underlying_type_t<Enum> to_underlying( Enum e ) noexcept {
Copy link
Contributor

@jake-at-work jake-at-work Mar 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am torn on this... I both like and dislike it.

Dislikes:

  • The name, underlying doesn't feel right, but I can't put my finger on it. to_ordinal?
  • It sort of undoes one of the key features of enum class by casting the enum something, you don't really see what, that can decay to another type.
  • Doesn't have a reverse like static_cast does so there is no mirror operation going back and forth between enum and ordinal value.

Like:

  • Hides the mess of casting to a int type.

🤷

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Counterpoints:

  • This is a direct copy of an STL utility we just don't have in C++11.
  • Look where I'm using it - at some point, we need to write these enum values to the byte stream. Now there are two ways we can do that:
    • We can map an enum to a value, and a value to an enum, like via some switch statement. If we do that, then we don't need to specify the enum values themselves, since we'd never cast the enum to or from an integer type. This is just redundant in every way.
    • We cast the enum where we write bytes to the wire.
  • It doesn't need a reverse. You can static cast any integer to an enum and the compiler will let you know if you're narrowing, but if you want to cast from an enum to the numeric type used to implement it, that's built into the enum type but you wouldn't know it, or can't rely on it since it can change, either by the compiler, or in the case of an enum class, the implementer. The compiler always knows, so this leverages deduction to get it right for you.

But I feel you. I don't like the use of enums overall. There's just something about them that don't seem "good enough". I don't think C++ would have had enumerations in the form we have them today if we didn't inherit from C, so I'm always aware of the loose type safety they come with.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I wasn't aware they added this in 14. Well then... if the C++ gods like it then we should too. Thanks!

return static_cast<underlying_type_t<Enum>>(e);
}

#endif