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

TBool -> TInt casts/conversions #736

Closed
jordens opened this issue May 22, 2017 · 20 comments
Closed

TBool -> TInt casts/conversions #736

jordens opened this issue May 22, 2017 · 20 comments

Comments

@jordens
Copy link
Member

jordens commented May 22, 2017

In CPython True << 1 == 2.

In ARTIQ Python:

<artiq>/coredevice/sawg.py:71:23-71:25: error: expected '<<' operand to be of integer type, not bool
                (clr2 << 1) | (clr0 << 2))
                 ~~~~ ^^

Explicit conversions are also broken:

int(True)
@jordens
Copy link
Member Author

jordens commented May 22, 2017

@sbourdeauducq
Copy link
Member

@jordens Can you live without that for PDQ3?

@jordens
Copy link
Member Author

jordens commented Jun 6, 2017

I can. But people won't like it. It's wrong. And I fear it will just end up on the pile of unresolved issues.

@sbourdeauducq sbourdeauducq modified the milestones: 4.0, 3.0 Jun 6, 2017
@sbourdeauducq
Copy link
Member

Sure, but considering the current situation I don't think we can fix that in time for 3.0.

@jordens
Copy link
Member Author

jordens commented Jun 6, 2017

I'd rather delay 3.0 than ship it with issues like these.

@sbourdeauducq
Copy link
Member

Is there someone else affected by this? @dhslichter @cjbe @jbqubit @hartytp @r-srinivas are you using Python booleans as integers?

@jordens
Copy link
Member Author

jordens commented Jun 6, 2017

@sbourdeauducq You will need to supply more context and explain the issue and the general consequences if you want to have a vote on this.

@sbourdeauducq
Copy link
Member

It's quite simple, in Python you can do integer operations on booleans directly, with True implicitly converted to 1 and False implicitly converted to 0, e.g.:

>>> True+True
2
>>> True+False
1
>>> 5*True
5

Currently in ARTIQ-Python this does not work and you get a type error; you must convert the booleans to integers explicitly instead.

@jordens
Copy link
Member Author

jordens commented Jun 6, 2017

There is more people should know:

  • If not fixed in 3.0 but later, APIs will be broken.
  • Leads to less readable code.
  • Is it OK to systematically delay/ignore-for-now all compiler issues and bugs where there are workarounds?

@sbourdeauducq
Copy link
Member

How will APIs be broken?

@jordens
Copy link
Member Author

jordens commented Jun 6, 2017

@kernel
    def set_clr(self, clr0: TInt32, clr1: TInt32, clr2: TInt32):
      ...

will become

@kernel
    def set_clr(self, clr0: TBool, clr1: TBool, clr2: TBool):
      ...

@sbourdeauducq
Copy link
Member

You can make it TBool right from the start, and put the bool to int conversion into your set_clr function.

@jordens
Copy link
Member Author

jordens commented Jun 6, 2017

IIRC explicit conversions are broken as well. There would need to be ifs...

@sbourdeauducq
Copy link
Member

Also it's harsh of you to say that compiler issues are "systematically" delayed; look at the commit log for April 22 to see some counter-examples.

@jordens
Copy link
Member Author

jordens commented Jun 6, 2017

I didn't say that! That was a policy question, not a blanket statement about the past. To me a compiler where I don't have to worry that much about hitting and then circumnavigating unexpectedly buggy or slow behavior is at the top of my wishlist. Let me rephrase it: How important are compiler bugs to people and can we ignore them for now if there are workarounds?

@jordens jordens changed the title implicit TBool -> TInt casts TBool <-> TInt casts Jun 6, 2017
@jordens jordens changed the title TBool <-> TInt casts TBool -> TInt casts/conversions Jun 6, 2017
@cjbe
Copy link
Contributor

cjbe commented Jun 6, 2017

@sbourdeauducq I have hit this before, however it is one of the more understandable compiler bugs to work around.

Things like #685 and #719 are more important to me in terms of removing recurring pain.

@dhslichter
Copy link
Contributor

dhslichter commented Jun 6, 2017

I am with @cjbe, things like #407, #685, and also #719 are more important to our day-to-day usage. I don't use booleans as integers, at least not in the way that @jordens is describing in the initial post. I think there is a balance to be struck between allowing for all the things Python does in ARTIQ Python, and making things too restrictive in ARTIQ Python, but this seems like a fairly simple thing for someone to figure out what's happening and adjust their code appropriately, and I guess I wouldn't delay the 3.0 release for this.

I think explicit conversions (e.g. int(True)) would be more important to handle than the general math cases (e.g. True << 3 or True + False), but it seems like modifying things to handle explicit conversions would end up meaning handling the more general stuff too, to avoid user confusion.

@r-srinivas
Copy link

I think the only time we've hit this is at some point we were setting up some chip selects with ttls and I think ttl.set_o(1) throws an error? Of course, one can work around this fairly easily.

@jbqubit
Copy link
Contributor

jbqubit commented Jun 6, 2017

I concur with @dhslichter and @cjbe.

@sbourdeauducq sbourdeauducq modified the milestones: 4.0, 5.0 Jan 11, 2018
@sbourdeauducq
Copy link
Member

On NAC3:

...
def run() -> int32:
    output_int32(True << 1)
    return 0

errors out with:

No such attribute __lshift__ at line 11 column 23

(and same thing with True + True / __add__, etc.)
We could implement those but it does not seem worthwhile; the general NAC3 design philosophy is to have explicit type conversions anyway.

Explicit conversions work:

def run() -> int32:
    output_int32(int32(True))
    output_int32(int32(False))
    output_int64(int64(True))
    output_int64(int64(False))
    return 0
1
0
1
0

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

No branches or pull requests

7 participants