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

Update admin.sma #1063

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Update admin.sma #1063

wants to merge 7 commits into from

Conversation

mlibre2
Copy link

@mlibre2 mlibre2 commented Sep 10, 2022

changelog

added

  • small optimization -> g_users_ini

edited

  • charsmax is now the protagonist
  • constants apply example: configsDir[MAX_RESOURCE_PATH_LENGTH]
  • unpack the code to make it more readable
  • rearrange empty spaces throughout the code

replaced

  • format -> formatex
  • console_print -> engclient_print

eliminated

  • all semicolons -> reference to this: #575
    We don't see why apply it without using #pragma semicolon 1

new variables already defined passed to static
fix macOS compiler
use max size according to amxconst
@luxxxoor
Copy link
Contributor

luxxxoor commented Jul 5, 2023

you have wrong indentation, also why static instead of new ? you keep memory used instead of freeing it

@mlibre2
Copy link
Author

mlibre2 commented Jul 5, 2023

you have wrong indentation, also why static instead of new ? you keep memory used instead of freeing it

  1. wrong indentation? Apple Build: NASM cert issue with CURL adversely affecting compile #1073
  2. static or new? https://forums.alliedmods.net/showpost.php?p=344418&postcount=1

@luxxxoor
Copy link
Contributor

luxxxoor commented Jul 5, 2023

you should only use static when its not feasible to use new

for example:

1.a function that is called multiple times in a second
2.you make a heavy operation and you don't want to repeat it every time

yes, static is faster than new, but you keep memory occupied, why would you need to keep memory occupied for a function that is call only once in a minute ?

just think why c/c++ or similar languages dont use global variables and instead prefer to use dynamic allocation

@mlibre2
Copy link
Author

mlibre2 commented Jul 6, 2023

we free, memory ;)
@rtxa
Copy link
Contributor

rtxa commented Jul 6, 2023

Well, memory is cheap today and static is useful in expensive operations, but still I see this PR trivial or really minimal priority because the changes will not bring any benefit because this code isn't doing anything expensive or not being called so many times. In any case, the use of constants instead of magic numbers is a great change and isn't mentioned in the PR.

Also, I see that swapping console_print() to engclient_print() is not a good change because the former has support for UTF-8 character which is useful in a multilingual context while I'm not that sure about engclient_print(). So by trying to bring a small optimization, you're maybe breaking something. I have notice too that the plugin isn't fully translated.

Other reason which the PR is not good is whitespaces changes and code formatting changes which obsfucates what you're really trying to fix/improve and less chances to get your PR merged. Those should be in another PR or in the final commit.

@mlibre2
Copy link
Author

mlibre2 commented Jul 14, 2023

well this is just the tip of the iceberg, to update all other plugins!

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