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

split 'nothing_special' of arti_invoke() into separate function #1168

Open
wants to merge 1 commit into
base: NetHack-3.7
Choose a base branch
from

Conversation

argrath
Copy link
Contributor

@argrath argrath commented Dec 7, 2023

No description provided.

@pat-rankin
Copy link

This could be simplified by making nothing_special() return 'ECMD_TIME' and its callers use 'return nothing_special()' instead of having separate return after each call.

@argrath
Copy link
Contributor Author

argrath commented Dec 8, 2023

Although it can reduce code size, I don't think it is 'simplify.'
nothing_special() is not relevant to the return value of arti_invoke(), so nothing_special() should not have any information about that.

@entrez
Copy link
Contributor

entrez commented Dec 8, 2023

I don't see the problem with the goto here. It consolidates the early return behavior without this massive proliferation of single-purpose functions that just print a message, are used within one other function, and do nothing except provide an excuse not to use gotos. A lot of these PRs feel to me like a sort of cargo cult goto removal because "gotos are bad" rather than fixing some serious readability issue this particular goto is causing.

@argrath
Copy link
Contributor Author

argrath commented Dec 9, 2023

I don't see the problem with the goto here.

No.
These gotos prevent further refactoring.

It consolidates the early return behavior without this massive proliferation of single-purpose functions that just print a message, are used within one other function, and do nothing except provide an excuse not to use gotos.

No.
After this PR is applied, we can split arti_invoke() easily.
I'll submit some following PRs when this PR is merged.

A lot of these PRs feel to me like a sort of cargo cult goto removal because "gotos are bad" rather than fixing some serious readability issue this particular goto is causing.

No.
My intention is not "gotos are bad", but "gotos (or anything) preventing further refactoring are bad."

@argrath
Copy link
Contributor Author

argrath commented Apr 4, 2024

Resolved conflicts and force-pushed.

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.

None yet

3 participants