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

Create constants for messages.conf and simplify switch job name switches #3265

Merged
merged 3 commits into from
Dec 30, 2023

Conversation

guilherme-gm
Copy link
Member

@guilherme-gm guilherme-gm commented Dec 22, 2023

Pull Request Prelude

Changes Proposed

1. Enum for messages.conf

Replacing the hardcoded numbers in source for conf/messages.conf entries with a enum.

This is meant to ease the readability a bit with constants that we can understand instead of numbers + comments.

I did not perform any check whether there are unused message entries (Although I do think there are).

I am open for suggestions about naming. Naming 1000+ entries in one go is very hard, so some stuff may have slipped with not so good names.

Huge thanks to @TioAkima who helped me naming all those enum entries.

For people upgrading their copies

  1. The messages.conf maximum value is now defined in common/msgtable.h, you can either add more items in there OR override it using a define:
#define MSGTBL_MAX 1600
  1. If you want to update your customizations to also use constants, a "hacky" conversion script is available here (Linux-only): https://github.com/guilherme-gm/Hercules-releases/tree/master/PR%20Extras/PR%203265%20-%20messages%20conf%20enum -- Check the folder readme for details

--

2. Using XMacro for JobID => Job Name conversions

I took the chance to also update the 2 functions in source that converts job ID to name. They had a hardcoded switch/case + message id, and I replaced both with XMacro using the class*.h files (as it is done in a few other places). This should help prevent desynchronization in the future.

Still about desynchronization, there were a few values missing in both functions, and with this PR these values are now correctly mapped.

I made a small plugin to dump all the values into the terminal, and it may be found here:
https://github.com/guilherme-gm/Hercules-releases/tree/master/PR%20Extras/PR%203265%20-%20messages%20conf%20enum/

It dumps the list of jobID => jobName so we can compare with comparison tools. A dump of master vs this PR is also available here: https://github.com/guilherme-gm/Hercules-releases/tree/master/PR%20Extras/PR%203265%20-%20messages%20conf%20enum/jobname_dump

For people upgrading their copies

If you have custom jobs, you may need to update the enum macros in src/common/class.h, src/common/class_special.h and src/common/class_hidden.h to include the message ID of your jobs.

Issues addressed:
Helps #165 (DON'T SOLVE IT)

@guilherme-gm guilherme-gm changed the title Create constants for messages.conf and switch job name switches to XMacro Create constants for messages.conf and simplify switch job name switches Dec 22, 2023
@4144
Copy link
Contributor

4144 commented Dec 22, 2023

In almost all lines like this
JOB_ENUM_VALUE(NOVICE, 0, MSGTBL_JOB_NOVICE)
probably better use for them simpler lines like this (MSGTBL_JOB_ can be added automatically)
JOB_ENUM_VALUE(NOVICE, 0)
and for exceptions use something like this
JOB_ENUM_VALUE2(THIRDJOB_BEGIN, 4053, MSGTBL_UNKNOWN_JOB)
?

@guilherme-gm
Copy link
Member Author

I was trying to make it more explicit so it doesn't hide too much info and gets confusing.

Those were my motivations:

  1. Being forced to always write all fields would make me think when adding a value: "Ok to declare a job I need to give it a name + id + message enum". While having the 2 variations I would try to only use name/id, and then get an error and would have to understand that I am missing an enum in msgtbl_.
  2. Also, when looking for where a enum value is being used, like, "who is using this MSGTBL_JOB_NOVICE? -- nothing, because it is hidden on the expansion", then I delete it and a include fails because of 2 levels of things being expanded ends up making it.

In my opinion X-macros are not too clear by default, but they do save a lot of code, so I am afraid of changing it like that and it becoming confusing.

But I can change if it is preferred to save those repetitions in the declaration.

@4144
Copy link
Contributor

4144 commented Dec 23, 2023

I think one macro is ok, but probably better remove prefix MSGTBL_ because for existing fields JOB_ and MAPID_ added automatically

@guilherme-gm
Copy link
Member Author

Ok. I still think it is adding unneeded abstraction, but it is much better than completely hiding them as the initial proposal. I am ok with this :) I will update the PR to remove MSGTBL_ prefix

@guilherme-gm guilherme-gm force-pushed the msgtable-enum branch 2 times, most recently from bce3420 to 596c20d Compare December 25, 2023 14:01
@MishimaHaruna MishimaHaruna added this to the Release v2023.12 milestone Dec 27, 2023
src/common/msgtable.h Outdated Show resolved Hide resolved
@MishimaHaruna MishimaHaruna merged commit 1e0f91f into HerculesWS:master Dec 30, 2023
254 of 256 checks passed
@guilherme-gm guilherme-gm deleted the msgtable-enum branch December 30, 2023 14:09
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