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

fix: Improve "/whitelist" syntax and use dynamic memory #296

Merged
merged 4 commits into from
Jan 26, 2025

Conversation

GitMensch
Copy link
Contributor

fixing a reference-out-of-bounds exception in case of "list" command and lots of whitelisted users (max: 100 users * 16 chars = 1600 with buffer being of size 255)

showcasing:

  • STRING ... WITH POINTER (that would also never write out of bounds and would allow an easy ON OVERFLOW EXIT PERFORM)
  • EVALUATE statement, including ALSO
  • mixed static and "if needed" dynamic memory

Warning: the initial reason was showcasing this use of EVALUATE, followed by recognition of better using STRING WITH POINTER, then realizing a not unlikely exception (first fixed by that ON OVERFLOW), then thinking about showcasing dynamic memory...

This PR is: totally untested (just checked that it compiles with GC 3.1, which needed some adjustments how ALLOCATE+FREE are used) and GC 3.2 and additional needs #294 to be merged up-front (if not wanted a MOVE SPACES TO DYN-BUFFER (BUFFER-POS:) instead of that SUBTRACT to get the length would be possible).

As noted this was mostly about showcasing those more rare used COBOL language features, feel free to pick only parts and/or adjust if others fall in the "less clear" or even "unmaintainable" state. The thing that likely should get in is the STRING (with ON OVERFLOW if the dynamic memory part is not used.

@meyfa
Copy link
Owner

meyfa commented Jan 25, 2025

The changes make sense to me, however there is a compilation error:

src/commands/whitelist.cob:102: error: 'BUFFER-ADDRESS' is not defined
  100 |                 *> overwrite the dynamic buffer's address with a dynamic..
  101 |                 IF DYN-BUFFER-LEN > LENGTH OF BUFFER
  102 >                    ALLOCATE DYN-BUFFER-LEN CHARACTERS RETURNING BUFFER-AD..
  103 |                    SET ADDRESS OF DYN-BUFFER TO BUFFER-ADDRESS
  104 |                 ELSE

Could you please take a look?

Also, it may seem wrong, but the message format "There are [n] whitelisted player(s)" was actually intentional. This is the exact message that the official Minecraft server sends (yes, even for n=1). Hence, this should be reverted.

@GitMensch
Copy link
Contributor Author

GitMensch commented Jan 26, 2025

fixed the missing variable, restored the strange but default "1 player(s): " message, rebased

fixing a reference-out-of-bounds exception in case of "list" command and lots of whitelisted users (max: 100 users * 16 chars = 1600 with buffer being of size 255)

showcasing:
* STRING ... WITH POINTER (that would also never write out of bounds and would allow an easy ON OVERFLOW EXIT PERFORM)
* EVALUATE statement, including ALSO
* mixed static and "if needed" dynamic memory
@meyfa meyfa changed the title whitelist adjustments fix: Improve "/whitelist" syntax and use dynamic memory Jan 26, 2025
@meyfa
Copy link
Owner

meyfa commented Jan 26, 2025

Hey @GitMensch, thanks for the contribution.

I've taken the liberty of reformatting the code a bit, and added #294 back in (this time with OPTIONAL). Tests look good with N=0, N=1, and N>1.

@meyfa meyfa merged commit 6be96fc into meyfa:main Jan 26, 2025
3 checks passed
@GitMensch GitMensch deleted the patch-3 branch January 26, 2025 15:39
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.

2 participants