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

feat(cppgc): add get and unwrap utlities to SameObject #1059

Merged

Conversation

petamoriken
Copy link
Contributor

These utilities are useful for implementing the Geometry Interface Module Level 1 (denoland/deno#27527).

@@ -182,10 +182,12 @@ impl FunctionTemplateData {
}
}

#[derive(Debug)]
Copy link
Member

Choose a reason for hiding this comment

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

the debug output is probably quite useless; any specific need for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intent is that this would allow #[derive(Debug)] to be added to DOMQuadInner, but I don't have a particularly strong opinion.

Copy link
Member

Choose a reason for hiding this comment

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

oh i see, that makes sense!

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 13 lines in your changes missing coverage. Please review.

Project coverage is 81.61%. Comparing base (0c7f83e) to head (7f40f07).
Report is 246 commits behind head on main.

Files with missing lines Patch % Lines
core/cppgc.rs 0.00% 13 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1059      +/-   ##
==========================================
+ Coverage   81.43%   81.61%   +0.17%     
==========================================
  Files          97      101       +4     
  Lines       23877    27862    +3985     
==========================================
+ Hits        19445    22740    +3295     
- Misses       4432     5122     +690     

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

@@ -212,4 +214,19 @@ impl<T: GarbageCollected + 'static> SameObject<T> {
})
.clone()
}

pub fn set(
Copy link
Member

Choose a reason for hiding this comment

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

when is this useful? sameobject should always just be set once and forget, any reason get cannot be used in your scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although get is sufficient, I think it may be useful when we want to assign a value in advance in a constructor or static method.
https://github.com/denoland/deno/blob/dfa4af12b093ac3a504d7fe98004ad7affd1c91a/ext/geometry/lib.rs#L311-L315

Copy link
Member

Choose a reason for hiding this comment

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

makes sense

Copy link
Member

@crowlKats crowlKats left a comment

Choose a reason for hiding this comment

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

LGTM

@crowlKats crowlKats merged commit 72820bf into denoland:main Jan 25, 2025
18 checks passed
@petamoriken petamoriken deleted the feat/add-coogc-same-object-utils branch January 25, 2025 00:12
@petamoriken petamoriken changed the title feat(webidl): add SameObject utils in cppgc feat(cppgc): add get and unwrap utlities to SameObject Jan 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants