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.

Sign up for GitHub

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

Jump to bottom

feat(hugr-core): Add pointer standard extension #1337

Merged
merged 4 commits into from
Jul 25, 2024
Merged

feat(hugr-core): Add pointer standard extension #1337

merged 4 commits into from
Jul 25, 2024

Conversation

ss2165
Copy link
Member

@ss2165 ss2165 commented Jul 23, 2024

ptr type and new, read and write operations.

@ss2165 ss2165 requested a review from doug-q July 23, 2024 15:37
@ss2165 ss2165 requested a review from a team as a code owner July 23, 2024 15:37
@ss2165 ss2165 mentioned this pull request Jul 23, 2024
Merged
@ss2165 ss2165 force-pushed the ss/hasdef branch 2 times, most recently from 84c065a to ed35c90 Compare July 24, 2024 09:27
doug-q
doug-q reviewed Jul 24, 2024
View reviewed changes
Copy link
Collaborator

@doug-q doug-q 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, will wait until parent is merged to approve.

hugr-core/src/std_extensions/ptr.rs Outdated
let ptr_t = ptr_type(TypeArg::new_var_use(0, TYPE_PARAMS[0].clone()));
let inner_t = Type::new_var_use(0, TypeBound::Copyable);
let body = match self {
PtrOpDef::New => Signature::new(vec![inner_t], vec![ptr_t]),
Copy link
Collaborator

Choose a reason for hiding this comment

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

we have impl From<Type> for TypeRow, so you don't need these vec!s.

hugr-core/src/std_extensions/ptr.rs Outdated
let inner_t = Type::new_var_use(0, TypeBound::Copyable);
let body = match self {
PtrOpDef::New => Signature::new(vec![inner_t], vec![ptr_t]),
PtrOpDef::Read => Signature::new(vec![ptr_t.clone()], vec![inner_t]),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think clone unnecessary

hugr-core/src/std_extensions/ptr.rs Outdated
/// Integer type of a given bit width (specified by the TypeArg).
///
/// Constructed from [ptr_custom_type].
pub fn ptr_type(ty: impl Into<TypeArg>) -> Type {
Copy link
Collaborator

Choose a reason for hiding this comment

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

when the TypeArg is for a TypeParam::Type we should take a Type or impl Into<Type>) here. There are two things the arg could be: a concrete Type, or a reference to some TypeParam, which is also represented as a Type.

hugr-core/src/std_extensions/ptr.rs Outdated
#[derive(Clone, Debug, PartialEq)]
/// A concrete pointer operation.
pub struct PtrOp {
def: PtrOpDef,
Copy link
Collaborator

Choose a reason for hiding this comment

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

you will get missing docs warnings here

Copy link
Member Author

Choose a reason for hiding this comment

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

i don't, probably because field is not pub?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wow, I wanted it to be pub so bad I assumed it was. Users must be able to see everything they need in an Op in order to be able to lower it.

Could you either make all these fields pub, or add accessors?

hugr-core/src/std_extensions/ptr.rs Outdated Show resolved Hide resolved
hugr-core/src/std_extensions/ptr.rs Outdated Show resolved Hide resolved
hugr-core/src/std_extensions/ptr.rs Outdated Show resolved Hide resolved
Base automatically changed from ss/hasdef to main July 25, 2024 09:03
@ss2165 ss2165 requested a review from doug-q July 25, 2024 09:06
doug-q
doug-q approved these changes Jul 25, 2024
View reviewed changes
Copy link
Collaborator

@doug-q doug-q left a comment

Choose a reason for hiding this comment

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

Perfect!

I pushed an empty commit to bump ci, lmk if you want me to rebase this away.

@ss2165 ss2165 added this pull request to the merge queue Jul 25, 2024
@codecov Codecov
Copy link

codecov bot commented Jul 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.52%. Comparing base (7ac015b) to head (b5ce8ba).

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #1337   +/-   ##
=======================================
  Coverage   87.52%   87.52%           
=======================================
  Files         111      111           
  Lines       19834    19834           
  Branches    17567    17567           
=======================================
  Hits        17360    17360           
  Misses       1699     1699           
  Partials      775      775
Flag Coverage Δ
python 91.53% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@doug-q doug-q linked an issue Jul 25, 2024 that may be closed by this pull request
Add ptr extension #1321
Closed
Merged via the queue into main with commit 88af215 Jul 25, 2024
22 checks passed
@ss2165 ss2165 deleted the ss/ptr branch July 25, 2024 09:14
@hugrbot hugrbot mentioned this pull request Jul 25, 2024
Merged
doug-q pushed a commit that referenced this pull request Jul 25, 2024
@ss2165
feat(hugr-core): HasDef and HasConcrete traits for def/concrete o…
7ac015b 
…p design pattern (#1336)

When there are custom types standing in for opdefs and concrete versions
of those ops, implementing these simple traits (how to instantiate one
from the other) allows them to be loaded from `CustomOp`.

Avoids some common boilerplate:
https://github.com/CQCL/tket2/blob/d9a713c068077dfea8c12301c1973575ddc8ca2c/tket2-hseries/src/extension/result.rs#L384
(in other extensions there too)

Used in #1337
github-merge-queue bot pushed a commit that referenced this pull request Jul 29, 2024
@hugrbot @doug-q
chore: release (#1330)
8499700 
## 🤖 New release
* `hugr`: 0.9.1 -> 0.10.0
* `hugr-core`: 0.6.1 -> 0.7.0
* `hugr-passes`: 0.6.1 -> 0.6.2
* `hugr-cli`: 0.2.1 -> 0.3.0

<details><summary><i><b>Changelog</b></i></summary><p>

## `hugr`
<blockquote>

## 0.10.0 (2024-07-26)

### Bug Fixes

(#1350))
- [**breaking**] Bump serialisation version with no upgrade path
([#1352](#1352))

### Features

- Add `nonlocal_edges` and `ensure_no_nonlocal_edges`
([#1345](#1345))
- Serialization upgrade path
([#1327](#1327))
- [**breaking**] Replace opaque type arguments with String
([#1328](#1328))
- Add `impl Hash for Type`
([#1347](#1347))
- `HasDef` and `HasConcrete` traits for def/concrete op design pattern
([#1336](#1336))
- Add pointer standard extension
([#1337](#1337))
- [**breaking**] Remove the `Eq` type bound.
([#1364](#1364))

### Refactor

- [**breaking**] Use JSON rather than YAML in opaque fields.
([#1338](#1338))
- [**breaking**] Declarative module behind optional feature flag
([#1341](#1341))

### Testing

- Miri gate serialization upgrades
([#1349](#1349))
</blockquote>

## `hugr-core`
<blockquote>

## 0.7.0 (2024-07-26)

### Bug Fixes

(#1350))
- [**breaking**] Bump serialisation version with no upgrade path
([#1352](#1352))

### Features

- Serialization upgrade path
([#1327](#1327))
- [**breaking**] Replace opaque type arguments with String
([#1328](#1328))
- Add `impl Hash for Type`
([#1347](#1347))
- `HasDef` and `HasConcrete` traits for def/concrete op design pattern
([#1336](#1336))
- Add pointer standard extension
([#1337](#1337))
- [**breaking**] Remove the `Eq` type bound.
([#1364](#1364))

### Refactor

- [**breaking**] Use JSON rather than YAML in opaque fields.
([#1338](#1338))
- [**breaking**] Declarative module behind optional feature flag
([#1341](#1341))

### Testing

- Miri gate serialization upgrades
([#1349](#1349))
</blockquote>

## `hugr-passes`
<blockquote>

## 0.6.2 (2024-07-26)

### Features

- Add `nonlocal_edges` and `ensure_no_nonlocal_edges`
([#1345](#1345))
</blockquote>

## `hugr-cli`
<blockquote>

## 0.3.0 (2024-07-26)

### Features

- [**breaking**] Created `validate` CLI subcommand.
([#1312](#1312))
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/MarcoIeni/release-plz/).

---------

Co-authored-by: Douglas Wilson <douglas.wilson@quantinuum.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@doug-q doug-q doug-q approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add ptr extension
2 participants
@ss2165 @doug-q