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

A strawfox for #417 to allow an interface to have a method which take… #418

Closed
wants to merge 1 commit into from

Conversation

mhammond
Copy link
Member

…s an Arc::clone(self)

[Had to do a last-minute rebase based on James's recent improvements and hope I didn't do anything even dumber there :]

Intent is a new attribute is added to such methods (and the method must be on
a [ThreadSafe] interface). I've chosen [SomethingSomethingArc] as the
attribute name :)

I attempted to change things so handle_map.rs no longer does the
ref-deref &* magic but instead delegates that to the macros, which
only does it if the new attribute exists. This nearly works, but
has unintended consequences I don't yet understand (ie, the
non-threadsafe generated code is using &*obj which makes no sense
at this hour).

…ch takes an `Arc::clone(self)`

Intent is a new attribute is added to such methods (and the method must be on
a `[ThreadSafe]` interface). I've chosen `[SomethingSomethingArc]` as the
attribute name :)

I attempted to change things so handle_map.rs no longer does the
ref-deref `&*` magic but instead delegates that to the macros, which
only does it if the new attribute exists. This *nearly* works, but
has unintended consequences I don't yet understand (ie, the
non-threadsafe generated code is using `&*obj` which makes no sense
at this hour).
@mhammond mhammond requested review from rfk and jhugman March 16, 2021 08:38
@@ -83,6 +86,11 @@ impl ThreadsafeCounter {
self.count.load(Ordering::SeqCst)
}
}

fn cloneable(self: Arc<Self>) {
Copy link
Member Author

Choose a reason for hiding this comment

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

to be clear, I think we want a new example rather than abusing this, but this is OK for demo purposes

Copy link
Contributor

@jhugman jhugman left a comment

Choose a reason for hiding this comment

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

Quick drive by. This looks straightforward enough. I'm sorry I'm not able to suggest any better user-facing names.

(i.e. the non-threadsafe generated code is using &*obj which makes no sense
at this hour).

I left a reason why this happening, but not a solution. :(

Comment on lines +69 to +73
{%- if meth.something_something_arc() -%}
let _retval = {{ obj.name() }}::{%- call to_rs_call_with_prefix("obj", meth) -%}?;
{%- else -%}
let _retval = {{ obj.name() }}::{%- call to_rs_call_with_prefix("&*obj", meth) -%}?;
{%- endif -%}
Copy link
Contributor

Choose a reason for hiding this comment

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

The method_call_with_result is doing the work of choosing between the two handle_map implementations, so this else block is being seen by both Threadsafe and non-Threadsafe objects.

pub(super) enum Attribute {
ByRef,
Error,
Threadsafe,
Throws(String),
SomethingSomethingArc,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really understand the need for this, so I'm not going to much help naming this.

@mhammond
Copy link
Member Author

I think #419 has longer legs than this, so I don't plan on taking this further.

@mhammond mhammond closed this Mar 18, 2021
@mhammond mhammond deleted the arc branch March 27, 2023 05:20
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.

2 participants