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

Exceptions ole #20

Open
wants to merge 47 commits into
base: main
Choose a base branch
from
Open

Exceptions ole #20

wants to merge 47 commits into from

Conversation

LithiumOx
Copy link
Member

No description provided.

olebol added 30 commits December 3, 2024 23:40
Copy link
Collaborator

@Max-Valk Max-Valk left a comment

Choose a reason for hiding this comment

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

revert alles behalve de extra checks en bot code

Copy link
Collaborator

Choose a reason for hiding this comment

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

verkeerde format gebruikt

Copy link
Collaborator

Choose a reason for hiding this comment

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

is wat make format doet

Copy link
Collaborator

Choose a reason for hiding this comment

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

verkeerde format

Copy link
Collaborator

Choose a reason for hiding this comment

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

is wat make format doet

Copy link
Collaborator

Choose a reason for hiding this comment

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

verkeerde format

Copy link
Collaborator

Choose a reason for hiding this comment

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

is wat make format doet

Copy link
Collaborator

Choose a reason for hiding this comment

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

onderdeel van een complete server rewrite die niet nodig is. zorg ervoor dat de extra checks voor de input in de oude code komen te staan in plaats van alle init functies op deze manier schrijven

Copy link
Collaborator

Choose a reason for hiding this comment

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

hier is letterlijk niks veranderd naast unused functies weggehaald, is alleen verschoven zodat ze bij elkaar staan

include/User.hpp Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

zet de sterretje if check waar je een empty get nickname kan verwachten ipv de const & weghalen voor alle getnickname calls

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ik heb dit uiteraard gecheckt en het moet overal sterretje zijn if empty. Which means bij elke getNickname checken en sterretje neerzetten OR gewoon bij deze sterretje returnen if empty.

Omdat je daardoor "*" returned kan je geen reference returnen, en aangezien het geen reference is boeit const helemaal niet.

Copy link
Collaborator

Choose a reason for hiding this comment

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

zoals al eerder vermeld is dit een compleet onnodige rewrite. deze hele file moet weg en alle changes erin moeten gerevert worden

Copy link
Collaborator

Choose a reason for hiding this comment

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

De meeste functies komen uit Server, heb ze apart getrokken omdat de Server file zeer groot was en ik dacht dat dit een goede verdeling en groepering was, ik kan die functies in principe terugzetten maar zie niet helemaal in waarom, het veranderd niks.

Ik heb checks toegevoegd op plekken waar deze nog niet stonden, in both environment en init functies, aangezien de oorspronkelijke versie een core dump issue had.

Ik heb de overall environment parsing inderdaad aangepast, de defines kunnen weg die zijn ondertussen redelijk nutteloos in de current file. Check next commit.

Ik heb toegevoegd dat command line arguments prioriteit krijgen over environment, omdat je deze manually zet. De env file heb ik prioriteit laten nemen over de system environment, aangezien je die ook zelf zet en je deze duidelijk liever wilt gebruiken.

Ik heb toegevoegd dat het werkt zonder arguments te passen aan het programma.

Ik heb de overall environment parsing code leesbaarder, duidelijker gemaakt en meer structuur gegeven, zodat onze evaluators het zo snel mogelijk kunnen lezen en wij zo snel mogelijk kunnen zien waar de fout zit. Ik zal een revisie doen in de volgende commit die het nog iets opschoont. Reverten is iig niet de oplossing, want er zitten echt verbeteringen in, die tested zijn.

src/User.cpp Outdated
@@ -74,7 +62,9 @@ void User::closeSocket() {

int User::getSocket() const { return this->socket; }

const string &User::getNickname() const { return this->nickname; }
string User::getNickname() const {
Copy link
Collaborator

Choose a reason for hiding this comment

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

again dit moet je niet veranderen in een van de meest gebruikte functies in heel de code waardoor nu iedere keer dat deze gecalled wordt een kopie van een string gemaakt wordt.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ik heb dit uiteraard gecheckt en het moet overal sterretje zijn if empty. Which means bij elke getNickname checken en sterretje neerzetten OR gewoon bij deze sterretje returnen if empty.

Omdat je daardoor "*" returned kan je geen reference returnen, en aangezien het geen reference is boeit const helemaal niet.

@@ -99,6 +89,9 @@ void User::setNickname(string &nickname) {
if (isdigit(nickname[0]) > 0) {
throw ErroneousNicknameException(nickname);
}
if (nickname == "*") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

gebruik hier gewoon nickname.empty()

Copy link
Collaborator

Choose a reason for hiding this comment

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

een extra parse functie die onnodig is en tevens een verkeerde result geeft als er geen message wordt meegestuurd. haal dit compleet weg

Copy link
Collaborator

Choose a reason for hiding this comment

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

Inderdaad fout gemaakt, gefixt zie revised

vind wel dat het in een andere functie moet en for_each overbodig complicated toevoegt

src/main.cpp Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

revert

Copy link
Collaborator

Choose a reason for hiding this comment

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

SIGKILL kan niet opgevangen worden.

De rest is veel overzichtelijker zo, maar kom vooral met suggesties

@olebol olebol self-assigned this Dec 20, 2024
Copy link
Collaborator

@olebol olebol left a comment

Choose a reason for hiding this comment

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

check new commits for changes and explanations

Copy link
Collaborator

Choose a reason for hiding this comment

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

is wat make format doet

Copy link
Collaborator

Choose a reason for hiding this comment

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

is wat make format doet

Copy link
Collaborator

Choose a reason for hiding this comment

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

is wat make format doet

Copy link
Collaborator

Choose a reason for hiding this comment

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

hier is letterlijk niks veranderd naast unused functies weggehaald, is alleen verschoven zodat ze bij elkaar staan

include/User.hpp Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ik heb dit uiteraard gecheckt en het moet overal sterretje zijn if empty. Which means bij elke getNickname checken en sterretje neerzetten OR gewoon bij deze sterretje returnen if empty.

Omdat je daardoor "*" returned kan je geen reference returnen, en aangezien het geen reference is boeit const helemaal niet.

this->addUser(clientSocket);
}

void Server::handleEvents(array<struct epoll_event, (size_t)ServerConfig::BACKLOG> &events, int numberOfEvents) {
for (int i = 0; i < numberOfEvents; i++) {
struct epoll_event &event = events[i];

if (event.data.fd == socket && (event.events & EPOLLIN)) {
if (event.data.fd == socket) {
if ((event.events & EPOLLIN) == 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Gecheckt en dit kan gebeuren maar betekent altijd dat het een error is, dus epoll error = server closen

Copy link
Collaborator

Choose a reason for hiding this comment

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

Het staat een beetje door elkaar for some reason door git, maar als je checkt met de file op main zijn de epoll functies precies hetzelfde met de uitzondering van de exceptions die nu bij errors worden gegooid ipv exits

Copy link
Collaborator

Choose a reason for hiding this comment

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

De meeste functies komen uit Server, heb ze apart getrokken omdat de Server file zeer groot was en ik dacht dat dit een goede verdeling en groepering was, ik kan die functies in principe terugzetten maar zie niet helemaal in waarom, het veranderd niks.

Ik heb checks toegevoegd op plekken waar deze nog niet stonden, in both environment en init functies, aangezien de oorspronkelijke versie een core dump issue had.

Ik heb de overall environment parsing inderdaad aangepast, de defines kunnen weg die zijn ondertussen redelijk nutteloos in de current file. Check next commit.

Ik heb toegevoegd dat command line arguments prioriteit krijgen over environment, omdat je deze manually zet. De env file heb ik prioriteit laten nemen over de system environment, aangezien je die ook zelf zet en je deze duidelijk liever wilt gebruiken.

Ik heb toegevoegd dat het werkt zonder arguments te passen aan het programma.

Ik heb de overall environment parsing code leesbaarder, duidelijker gemaakt en meer structuur gegeven, zodat onze evaluators het zo snel mogelijk kunnen lezen en wij zo snel mogelijk kunnen zien waar de fout zit. Ik zal een revisie doen in de volgende commit die het nog iets opschoont. Reverten is iig niet de oplossing, want er zitten echt verbeteringen in, die tested zijn.

src/Env.cpp Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

zie comment onder serverinit.cpp

src/main.cpp Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

SIGKILL kan niet opgevangen worden.

De rest is veel overzichtelijker zo, maar kom vooral met suggesties

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.

3 participants