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

remove seek from NetworkMessage #4891

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

remove seek from NetworkMessage #4891

wants to merge 1 commit into from

Conversation

xyron98
Copy link

@xyron98 xyron98 commented Feb 2, 2025

Pull Request Prelude

Changes Proposed

Remove seek functionality from NetworkMessage since it is not used anywhere in the codebase and can behave unexpectedly when used with addByte.

Issues addressed:

#4877

How to test:

Compile and run the server.

@ArturKnopik
Copy link
Contributor

probably

forgottenserver/src/networkmessage.h

Line 125 in b227678

bool setBufferPosition(MsgSize_t pos)
also can be removed, needs to be checked

@ArturKnopik ArturKnopik linked an issue Feb 3, 2025 that may be closed by this pull request
[Bug]: NetworkMessage always increase size even when it's not needed #4877
Open
4 tasks
@MillhioreBT
Copy link
Contributor

While it is true that the base does not use seek, you cannot be certain that someone else might use it. It is unsafe if used incorrectly, just like countless other things.

@MillhioreBT MillhioreBT requested a review from ranisalt February 4, 2025 21:15
@ranisalt
Copy link
Member

If seek is not used, tell is likely not used too

@Codinablack
Copy link
Contributor

Codinablack commented Feb 11, 2025 •
edited
Loading

So the point is to just remove lua API from the end users because its not used in the datapack?

@MillhioreBT
Copy link
Contributor

So the point is to just remove lua API from the end users is its not used in the datapack?

I thought that, in that case we must eliminate 50% of the LUA functions/methods that are not used in the Datapack
modalwindow, helper, event_helper, compat.lua, ect...

@MillhioreBT MillhioreBT requested a review from nekiro February 11, 2025 04:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ranisalt ranisalt Awaiting requested review from ranisalt

@nekiro nekiro Awaiting requested review from nekiro

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: NetworkMessage always increase size even when it's not needed
5 participants
@xyron98 @ArturKnopik @MillhioreBT @ranisalt @Codinablack