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

Join rule event #862

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

Join rule event #862

wants to merge 2 commits into from

Conversation

nvrWhere
Copy link
Collaborator

Upstream join rule event from neochat

@nvrWhere nvrWhere requested a review from KitsuneRal January 26, 2025 17:07
Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

Overall LGTM except a couple of technical things.

QString roomId;
QString type;
};
Q_NAMESPACE_EXPORT(QUOTIENT_API)
Copy link
Member

Choose a reason for hiding this comment

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

That doesn't quite work, unfortunately, because another header file might also need it and then we will end up with ODR violation at linking. For that reason, all namespace-level enumerations that need Q_ENUM are defined in quotient_common.h; it doesn't have Q_NAMESPACE_EXPORT for EventContent but I would argue that enum JoinRule is no different from, say, enum Membership and belongs to the main Quotient namespace.

};

//! Enum representing the available room join rules
enum JoinRule {
Copy link
Member

Choose a reason for hiding this comment

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

Always make sure your enums specify the underlying integer type. The standard leaves it for implementations to decide, so MSVC uses int if the type is omitted while GCC and both flavours of Clang use unsigned int - this is why the CI is failing on Windows now. I added the same integer type enum Membership uses but if you prefer uint8_t I won't mind :)

Suggested change
enum JoinRule {
enum JoinRule : uint16_t {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In work
Development

Successfully merging this pull request may close these issues.

2 participants