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

Respell Move::None and Move::Null to reflect that they're constants #5206

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dubslow
Copy link
Contributor

@dubslow dubslow commented May 1, 2024

…not functions

Despite the syntax, these are compiletime singleton constants, or rather they would be if such syntax was permitted:

static constexpr Move None = Move(0);
static constexpr Move Null = Move(65);

...but this syntax not permitted. In some ways this is a flaw of the language, but that's offtopic (and a gigantic can of worms best left sealed). So function syntax is what we're stuck with, but we can respell them to reflect that they really are just constants.

Also move MoveType under its explanatory comments and remove some blank lines

No functional change

…not functions

Despite the syntax, these are compiletime singleton constants, or rather they *would* be if such syntax was permitted:
```
static constexpr Move None = Move(0);
static constexpr Move Null = Move(65);
```
...but this syntax not permitted. In some ways this is a flaw of the language, but that's offtopic (and a gigantic can of worms best left sealed).
So function syntax is what we're stuck with, but we can respell them to reflect that they really are just constants.

Also move MoveType under its explanatory comments and remove some blank lines

No functional change
Copy link

github-actions bot commented May 1, 2024

clang-format 17 needs to be run on this PR.
If you do not have clang-format installed, the maintainer will run it when merging.
For the exact version please see https://packages.ubuntu.com/mantic/clang-format-17.

(execution 8909920007 / attempt 1)

@vondele
Copy link
Member

vondele commented May 5, 2024

I don't think this is a good enough reason to deviate from our usual convention on function names.

@dubslow
Copy link
Contributor Author

dubslow commented May 5, 2024

These aren't function names tho.

@MinetaS
Copy link
Contributor

MinetaS commented May 5, 2024

I haven't seen using a different convention for method names only because they represent constants...

@peregrineshahin
Copy link
Contributor

I haven't seen using a different convention for method names only because they represent constants...

I guess the unconventional part is to represent constants as functions to begin with in such a not so "functional programming language" but what do we know..

@dubslow
Copy link
Contributor Author

dubslow commented May 5, 2024

I haven't seen using a different convention for method names only because they represent constants...

How the heck often do you see methods representing constants????? That's a Bad Idea™ imo, an anti-pattern to be avoided.

The only reason it occurs here is because of C++ limitations (can of worms etc etc)

@peregrineshahin
Copy link
Contributor

The only reason it occurs here is because of C++ limitations (can of worms etc etc)

It has nothing to do with limitations, it can be easily done in the same soul the refactor did..
We have enum move_type, the same thing can be made as enum special_moves.

@dubslow
Copy link
Contributor Author

dubslow commented May 5, 2024

It has nothing to do with limitations, it can be easily done in the same soul the refactor did..
We have enum move_type, the same thing can be made as enum special_moves.

As I told Disservin on Discord, I do think there is much value to having these singleton constants in the Move namespace, however putting them in a separate enum with a similar pseudonamespace might be a better solution...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants