Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Dataflow analysis framework #1476
base: main
Are you sure you want to change the base?
feat: Dataflow analysis framework #1476
Changes from 167 commits
c7a9d89
ac45e53
8adaa6e
098c735
706c892
63bc944
5fa7edb
98bf94a
295ec32
5c8289e
bf173ab
0ae4d19
8608ba9
2dca3e9
51e68ea
8635474
80d5b86
1c8be99
b0afa54
d09a1fe
af8827b
4b61436
8b31d8c
6a72961
780af9b
cabcf04
5e4a04f
1453886
221e96c
cd4e15c
c5ab2a9
6c80acf
636f14d
0acdcc5
1012e0a
6bd8dba
f7d288f
a62eb0f
f0ec237
82a3f22
e6dc114
03ff165
25ed1fb
09911df
248fb07
e2ad079
95e2dd9
7f1e122
faff556
401354d
c468387
88db5b1
738b61b
8f9c1ed
a71ba97
05280a8
96b0856
f21e278
ce53b1c
13f29a9
9f1a5cd
15e642e
514af13
5d86f46
d8c8140
1315685
2aaaeb9
5619761
f3c175c
aad2ef0
0a8cc12
bfcd0a6
248fb23
5b8654e
c40e718
8732a63
346187d
a139f9e
7f2a91a
1680829
2a57a15
fcfcb6b
dcaa928
bcacbcc
5192ed8
e21bbd7
22e0192
cae5e4f
3014827
64b9bb7
8bc5e12
a3a6213
a96ab20
3051183
777694c
5a16e6b
4f31178
2b523c9
1d2cb9b
ee91bbe
e67051f
94cee55
5cf5ff0
7381087
60e33db
ef4f433
5935489
b198681
ed30f80
3f7808a
436b635
0374d13
6a2dd9e
a75fee9
151e571
e15b04d
dc08f0d
436dcd2
e817bbe
b06cfad
fd717be
9b17439
846d1ee
328e7f8
da3c05c
6930ad4
0a3e281
b1e0bfd
355e814
22f3ce8
68b1d48
77a5fa3
2cc62f0
8254771
7e81b15
34e82ed
015707f
ff39f7d
ada7ee1
7c02d41
3d4f016
8bab4d5
3eccadf
0f4fa52
caa8aca
e7f61fc
811802c
3713ea7
d317809
69c3270
dc56686
33a8592
b153ada
5052ac0
ad0c6f2
16a18f4
a0f2b2c
87eb700
a49221b
71ea55d
ea9db2e
df31523
a490874
3af39aa
dc15999
2d81264
b61d252
8cac194
fb3816e
19571f6
69d0f5e
cb60b5b
2624ee8
ec526e8
5650ee4
da2981c
0b68454
4916e9d
a4a64c0
bc39b76
92669db
33c8607
97496f9
8b76135
c18cbea
1b64b4b
39b8df1
a5d987c
3e718fd
497686a
e34c7be
69a69f3
57ac432
f9a9f24
9cc368d
a61fbdb
24cce0e
a590766
2b2c461
124718d
93b1f4d
731a3b0
7040e83
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatives include
a. moving
value_from_const
out of the trait and fixing it; I don't have a concrete reason why anyone would need to override it but it seems "reasonable" that one might; orb. moving
value_from_opaque
,value_from_const_hugr
andvalue_from_function
into a separate trait (sayConstLeafLoader
), and then implementingConstLoader
for any type providing those two - this would be the more proper way of doing what I've done here but feels a bit overkill TBH.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't these functions just return
Top
directly?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could make it
PartialValue
but then that allows returning lots of things that make no sense at all, likeBottom
("can't happen", what, yes it can) or of course aSum
- when we know it isn't one...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
fields
args in these methods need some documentation. I'm not sure what they representThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched to
ConstLocation
in the hope that is clearer thannode
/fields
. LMK if still unclear!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't this a method on
ConstLoader
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's got an extra param vs the version on
ConstLoader
but TBH I am leaning towards removingConstLoader::value_from_const
(keeping just the "leaf" methodsvalue_from_opaque
,value_from_const_hugr
andvalue_from_function
) - assuming we want correct treatment of sums for conditionals, MakeTuple, etc. etc., every valid impl has to do this....There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, gone in circles here, but after introducing ConstLocation, I then wanted
impl Into<ConstLocation>
which you can't do on a trait. So I've made it an external method, and we can add an extra trait method (defaulting to the external) as a non-breaking change if we want...