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

Chat server #39

Closed
wants to merge 4 commits into from
Closed

Chat server #39

wants to merge 4 commits into from

Conversation

Vicx95
Copy link
Collaborator

@Vicx95 Vicx95 commented Apr 18, 2024

No description provided.

@ziobron ziobron added the Standard The user has Standard course option label Apr 18, 2024
Copy link
Contributor

@lukasz126 lukasz126 left a comment

Choose a reason for hiding this comment

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

I think we shouldn't use :unique_ptr to raw pointers, this may cause memory problems

qInfo() << "Server listening on port " << port;
connect(m_webSocketServer, &QWebSocketServer::newConnection, this, &ChatServer::onNewConnection);
connect(&*m_webSocketServer, &QWebSocketServer::newConnection, this, &ChatServer::onNewConnection);
Copy link
Contributor

Choose a reason for hiding this comment

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

please use m_webSocketServer.get()

server/chatserver.cpp Show resolved Hide resolved
}

ChatServer::ChatServer(quint16 port, QObject *parent)
ChatServer::ChatServer(quint16 port, QObject* parent)
Copy link
Contributor

Choose a reason for hiding this comment

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

const?

QWebSocketServer* m_webSocketServer;
QList<QWebSocket*> m_clients;
std::unique_ptr<QWebSocketServer> m_webSocketServer;
std::vector<std::unique_ptr<QWebSocket*>> m_clients;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we shouldn't use std::vector<std::unique_ptr to raw pointers, this may cause memory problems

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you have other idea? We have to store QWebSocket*, there is no option to store as a normal type

@ziobron
Copy link
Contributor

ziobron commented Apr 26, 2024

@Vicx95 Your solution has not passed all checks (maybe it was only a formatting-check?) and you haven't modified this Pull Request during the last 7 days. It will be automatically closed after the next 7 days if it remains untouched.

1 similar comment
@ziobron
Copy link
Contributor

ziobron commented May 4, 2024

@Vicx95 Your solution has not passed all checks (maybe it was only a formatting-check?) and you haven't modified this Pull Request during the last 7 days. It will be automatically closed after the next 7 days if it remains untouched.

@ziobron
Copy link
Contributor

ziobron commented May 10, 2024

We are closing this PR. If you want to fix your code and get points, please re-open it or raise a new PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auto-closed Standard The user has Standard course option
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants