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

EventManager/Eventful - Generate event arrays #2097

Merged
merged 4 commits into from
Apr 15, 2022

Conversation

cppcooper
Copy link
Contributor

A PR for just the auto-generated arrays part so we can avoid manual synchronization and sneaky problems arising.

@myk002
Copy link
Member

myk002 commented Apr 14, 2022

legitimate build error on windows:
library/modules/EventManager.cpp(186): error C2027: use of undefined type 'std::array<eventManager_t,16>'

@cppcooper
Copy link
Contributor Author

Oops. I forgot the include

@myk002
Copy link
Member

myk002 commented Apr 15, 2022

Overall, looks good. Have you sanity checked that appropriate events get triggered with devel/eventful-client?

Edit: it occurs to me that eventful-client should really be dumping the args that get passed to the handlers. I'll get that in when I get the chance

@cppcooper
Copy link
Contributor Author

appropriate events get triggered with devel/eventful-client

I checked a JOB_INITIATED event 🤷 and that's what it said in the console. But also I'm not sure how it would trigger the wrong events based on how the array is built. It iterates the enum, then uses a switch to pick the function corresponding to that enum value so I'd have to type the wrong function in the switch for the wrong events to be triggered.

@myk002
Copy link
Member

myk002 commented Apr 15, 2022

so I'd have to type the wrong function in the switch for the wrong events to be triggered.

A good rule to program by: code that isn't tested is broken. Just because it looks obviously right doesn't mean that it is. Always challenge your assumptions. No one person can understand everything in a complex system, and DF (and DFHack) is a complex system. Corner cases can surprise you.

@myk002
Copy link
Member

myk002 commented Apr 15, 2022

I'm not sure if you have a usual process for this already, but I've found that the best way to continue development after splitting off some code like this is to create a diff patch between your dev branches and then apply the diff to a new branch off develop once the split-off code is merged. E.g:

git diff fix-2301 generate-event-arrays >/tmp/em.diff
git checkout develop; git fetch upstream; git merge upstream/develop
git checkout -b new-EventManager
git apply /tmp/em.diff

@myk002 myk002 merged commit 9bf9a79 into DFHack:develop Apr 15, 2022
@cppcooper
Copy link
Contributor Author

I'm not sure if you have a usual process for this already, but I've found that the best way to continue development after splitting off some code like this is to create a diff patch between your dev branches and then apply the diff to a new branch off develop once the split-off code is merged. E.g:

git diff fix-2301 generate-event-arrays >/tmp/em.diff
git checkout develop; git fetch upstream; git merge upstream/develop
git checkout -b new-EventManager
git apply /tmp/em.diff

What's wrong with merge? or rebase

@myk002
Copy link
Member

myk002 commented Apr 15, 2022

If that works, great. I find it gives me a lot of merge conflicts when I do it that way. Creating a diff and repatching allows you to only care about what the text looks like and not have to care about the commit chain that got it there.

@cppcooper cppcooper deleted the generate-event-arrays branch April 19, 2022 05:35
@lethosor lethosor added this to In Progress in 0.47.05-r5 via automation May 9, 2022
@lethosor lethosor moved this from In Progress to Done in 0.47.05-r5 May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants