realXtend / tundra

realXtend Tundra SDK, a 3D virtual world application platform.
www.realxtend.org
Apache License 2.0
84 stars 70 forks source link

Not all connection IDs were changed from int to uint #568

Closed jonnenauha closed 11 years ago

jonnenauha commented 11 years ago

UserConnection

There was this recent port to make everything having user connection id:s from u8 at the protocol level and from int at the other code level to u32. I noticed I get warning on some of my code as TundraProtocolModules UserConnection class still has a getter of ConnectionId() returning int. The member is correctly u32 userID but auto casting is done on the getter. This is not a big deal really as it will only affect servers/clients that get mid way of uint before anything will start to break :)

Anyways to finish off the conversion work it should be changed.

@cvetan5 Could do this when he has time as did the earlier work also :)

/// Represents a client connection on the server side.
class TUNDRAPROTOCOL_MODULE_API UserConnection : public QObject, public boost::enable_shared_from_this
{
    Q_OBJECT
    Q_PROPERTY(int id READ ConnectionId)

public:
    UserConnection() : userID(0) {}

    /// Returns the connection ID.
    int ConnectionId() const { return userID; }

    /// Message connection
    Ptr(kNet::MessageConnection) connection;
    // Connection ID
    u32 userID;

EC_WebView

UserDisconnected signal now has uint as param, these wont produce build errors but will produce runtime fails to connect.

connect(server, SIGNAL(UserDisconnected(int, UserConnection*)), SLOT(ServerHandleDisconnect(int, UserConnection*)), Qt::UniqueConnection); needs to be change to uint.

void EC_WebView::ServerInitialize(TundraLogic::Server *server)
{
    if (!server || !server->IsRunning())
        return;
    connect(server, SIGNAL(UserDisconnected(int, UserConnection*)), SLOT(ServerHandleDisconnect(int, UserConnection*)), Qt::UniqueConnection);
    connect(this, SIGNAL(AttributeChanged(IAttribute*, AttributeChange::Type)), SLOT(ServerHandleAttributeChange(IAttribute*, AttributeChange::Type)), Qt::UniqueConnection);
}
Stinkfist0 commented 11 years ago

All signatures should use u32, see this comment https://github.com/realXtend/naali/pull/562#issuecomment-10452454

jonnenauha commented 11 years ago

Dont really care what typedef is used, as long as they are consistent.

Stinkfist0 commented 11 years ago

Ok. Anyways I care, because I'm interested in doing things correctly. Luckily usually making things correctly instead of hacking means that the results are consistent also so we both should be happy.

peterclemenko commented 11 years ago

For 64 bit compatibility it may be better to use a size_t or uintptr_t. See http://www.viva64.com/en/a/0050/ for details.

Stinkfist0 commented 11 years ago

Usage of size_t is not desired here. U32 allows user count that is more than enough and is guaranteed to have constant range on different platforms, which prevents weird (although mostly theoretical) bugs f.ex. when using 32-bit client on a 64-bit server.

peterclemenko commented 11 years ago

Read the article again. It is still a uint, however it uses the, 32 or 64 bit memory allocation based on the build architecture. It is actually faster on 64 bit due to proper 64 bit allocation being used.

"Ali Kämäräinen" notifications@github.com wrote:

Usage of size_t is not wanted here. U32 allows user count that is more than enough and is guaranteed to have constant range regardless whether we're building 32-bit or 64-bit application.


Reply to this email directly or view it on GitHub: https://github.com/realXtend/naali/issues/568#issuecomment-10806296

Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Stinkfist0 commented 11 years ago

Usage of size_t is not needed as we're not dealing with memory-related quantities here.

jonnenauha commented 11 years ago

Someone really should get on this and change everything to u32... :P