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

Add logical XOR to GDScript #76191

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

Conversation

JeffVenancius
Copy link
Contributor

@JeffVenancius JeffVenancius commented Apr 18, 2023

This is an incomplete implementation of a logical XOR.
I've used 'but' and '^^' as keyword and aliases. It works but only if you do one comparison at a time.
There's no highlighting for 'but' and there's no auto-completion.
I'll keep working on it, still.

@JeffVenancius JeffVenancius requested review from a team as code owners April 18, 2023 02:15
@MewPurPur
Copy link
Contributor

You have not linked a proposal suggesting this or defended your case. For booleans, != does the same as XOR afaik.

@Calinou
Copy link
Member

Calinou commented Apr 18, 2023

You have not linked a proposal suggesting this or defended your case.

There's already an open proposal: godotengine/godot-proposals#3849

It hasn't garnered much community support, and the original idea was already rejected more than once.

@dalexeev
Copy link
Member

I've used 'but' and '^^' as keyword and aliases.

Why but? I think xor is more familiar and expected.

@YuriSizov YuriSizov changed the title logiocal XOR on gdscript Add logical XOR to GDScript Apr 18, 2023
@JeffVenancius
Copy link
Contributor Author

I've used 'but' and '^^' as keyword and aliases.

Why but? I think xor is more familiar and expected.

I've choosed "^^" because I thought it would make sense aliases-wise.
You know, as logical and is "&&" and logical or is "||".
I don't mind changing it to XOR, if it's prefered or even put it as one more alias for it.

@JeffVenancius
Copy link
Contributor Author

JeffVenancius commented Apr 18, 2023

You have not linked a proposal suggesting this or defended your case. For booleans, != does the same as XOR afaik.

Sorry about that, I'll look into it!

It's not the same thing, though, I mean...

var foo = 0
var bar = 1

if foo == 0 or bar == 1:
    pass # will execute even if you don't want to.

if foo == 0 and bar != 1 or foo != 0 and bar == 1:
    pass # will not not execute. 

if foo == 0 and bar != 2 or foo != 2 and bar == 1:
    pass # will execute.

# My proposal shortens this into this one:
if foo == 0 but bar == 1:
    pass # you can read it left to right or right to left.

@JeffVenancius JeffVenancius force-pushed the but_statement_on_gdscript branch 3 times, most recently from 5070632 to dec3364 Compare April 19, 2023 01:06
@0x0ACB
Copy link
Contributor

0x0ACB commented Apr 20, 2023

You have not linked a proposal suggesting this or defended your case. For booleans, != does the same as XOR afaik.

Sorry about that, I'll look into it!

It's not the same thing, though, I mean...

var foo = 0
var bar = 1

if foo == 0 or bar == 1:
    pass # will execute even if you don't want to.

if foo == 0 and bar != 1 or foo != 0 and bar == 1:
    pass # will not not execute. 

if foo == 0 and bar != 2 or foo != 2 and bar == 1:
    pass # will execute.

# My proposal shortens this into this one:
if foo == 0 but bar == 1:
    pass # you can read it left to right or right to left.
if foo != bar:
  pass # will execute

shortened this even more for you ^^. For booleans != is equivalent to xor. Or if I am misunderstanding your example and the booleans are supposed to be the foo == 0 part:

if (foo == 0) != (bar == 1)

Truth table for !=

a b res
false false false
false true true
true false true
true true false

Truth table for xor

a b res
false false false
false true true
true false true
true true false

@JeffVenancius
Copy link
Contributor Author

You have not linked a proposal suggesting this or defended your case. For booleans, != does the same as XOR afaik.

Sorry about that, I'll look into it!
It's not the same thing, though, I mean...

var foo = 0
var bar = 1

if foo == 0 or bar == 1:
    pass # will execute even if you don't want to.

if foo == 0 and bar != 1 or foo != 0 and bar == 1:
    pass # will not not execute. 

if foo == 0 and bar != 2 or foo != 2 and bar == 1:
    pass # will execute.

# My proposal shortens this into this one:
if foo == 0 but bar == 1:
    pass # you can read it left to right or right to left.
if foo != bar:
  pass # will execute

shortened this even more for you ^^. For booleans != is equivalent to xor. Or if I am misunderstanding your example and the booleans are supposed to be the foo == 0 part:

if (foo == 0) != (bar == 1)

Truth table for !=
a b res
false false false
false true true
true false true
true true false

Truth table for xor
a b res
false false false
false true true
true false true
true true false

You're right!
Actually, the last implementation I've made is basically this, but with a different precedence - which means you won't need parenthesis, here:

if (foo == 0) != (bar == 1) # usual.

if foo == 0 but bar == 1 # my implementation. The "but" is checked after the other ones.

core/variant/variant.h Outdated Show resolved Hide resolved
modules/gdscript/gdscript_parser.h Outdated Show resolved Hide resolved
modules/gdscript/gdscript_tokenizer.cpp Outdated Show resolved Hide resolved
@dalexeev
Copy link
Member

There's no highlighting for 'but' and there's no auto-completion.

For autocompletion you need to add the keyword here:

static const char *_keywords_with_space[] = {
"and", "not", "or", "in", "as", "class", "class_name", "extends", "is", "func", "signal", "await",
"const", "enum", "static", "var", "if", "elif", "else", "for", "match", "while",
nullptr
};

@JeffVenancius JeffVenancius requested review from a team as code owners April 27, 2023 21:13
@YuriSizov YuriSizov removed the request for review from a team April 27, 2023 22:52
@JeffVenancius JeffVenancius force-pushed the but_statement_on_gdscript branch 5 times, most recently from d63b799 to 10ecc1a Compare April 28, 2023 01:35
@JeffVenancius
Copy link
Contributor Author

I think checks are not working because of documentation but I don't know what to do, atleast not right now.
That's the only thing left, though.

@JeffVenancius JeffVenancius force-pushed the but_statement_on_gdscript branch 2 times, most recently from d2e7994 to 50102f7 Compare April 28, 2023 03:16
@dalexeev
Copy link
Member

I think checks are not working because of documentation but I don't know what to do, atleast not right now.

You can use the bin/<godot executable> --doctool command to regenerate the documentation. But in this case the error is different:

modules/noise/noise_texture_3d.cpp Outdated Show resolved Hide resolved
modules/gdscript/gdscript_tokenizer.cpp Outdated Show resolved Hide resolved
modules/gdscript/gdscript_parser.h Outdated Show resolved Hide resolved
modules/gdscript/gdscript_tokenizer.cpp Outdated Show resolved Hide resolved
@JeffVenancius JeffVenancius force-pushed the but_statement_on_gdscript branch 2 times, most recently from e50c4d5 to 22a2a06 Compare April 28, 2023 19:34
Copy link
Member

@dalexeev dalexeev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added some changes in addition to yours. You can apply the patch with git apply or git am. Please note that this PR should have one commit (not multiple). If you want, you can add me as a co-author, but it doesn't really matter to me.

xor_review.patch.txt

P.S. Sorry for the long reply, I was sick and couldn't review your PR.

@JeffVenancius JeffVenancius force-pushed the but_statement_on_gdscript branch 2 times, most recently from ca1245c to 930832b Compare May 5, 2023 17:36
applied requested changes
Co-authored-by: Danil Alexeev <[email protected]>
@JeffVenancius
Copy link
Contributor Author

I have added some changes in addition to yours. You can apply the patch with git apply or git am. Please note that this PR should have one commit (not multiple). If you want, you can add me as a co-author, but it doesn't really matter to me.

xor_review.patch.txt

P.S. Sorry for the long reply, I was sick and couldn't review your PR.

That's OK, thank you for your patience and help!

Copy link
Member

@dalexeev dalexeev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@adamscott adamscott requested a review from vnen January 5, 2024 14:33
@adamscott
Copy link
Member

@JeffVenancius If you could rebase your PR, we could merge it soon. :)

@vnen
Copy link
Member

vnen commented Jan 5, 2024

@JeffVenancius If you could rebase your PR, we could merge it soon. :)

Well, perhaps we should discuss this with the whole GDScript team, given the proposals for this are controversial.

Even so, this PR cannot be merged as is because it only adds the keyword to the language, it does not implement the actual operator at runtime. It would also be nice to add tests, to verify the implementation itself.

@shenkes
Copy link

shenkes commented Sep 16, 2024

FWIW, this PR seems related to the boolean operators section of the style guide where it is encouraged to use AND, OR, and NOT instead of &&, ||, and !

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.

Add XOR operator to GDScript
9 participants