M-Labs/nac3
M-Labs
/
nac3
8
0
Fork 5
Code Issues 89 Pull Requests 7 Releases Wiki Activity

WIP: support_string_class_attr #415

Closed
abdul124 wants to merge 2 commits from support_string_class_attr into master
pull from: support_string_class_attr
merge into: M-Labs:master
Conversation 2 Commits 2 Files Changed 4 +75 -7
abdul124 commented 2024-06-13 15:55:11 +08:00
Collaborator
Copy Link

The main changes are as follows:
nac3artiq/src/lib.rs:

Added String to the list of primitive attributes

nac3artiq/src/symbol_resolver.rs

Updated Constructor function detection in get_obj_type function to avoid deadlock when performing read on definitions
Allow classes registered using nac3 decorator to be used in kernel function
Added support for String Attributes #337

Also added two files under nac3artiq/demo for testing the new changes

The main changes are as follows: **nac3artiq/src/lib.rs:** > Added String to the list of primitive attributes **nac3artiq/src/symbol_resolver.rs** > Updated Constructor function detection in _get_obj_type_ function to avoid deadlock when performing read on definitions > Allow classes registered using nac3 decorator to be used in kernel function > Added support for String Attributes #337 Also added two files under nac3artiq/demo for testing the new changes
abdul124 added 7 commits 2024-06-13 15:55:11 +08:00
abdul124 force-pushed support_string_class_attr from b3b34481d4 to 2470ab7856 2024-06-13 16:40:11 +08:00 Compare
abdul124 added 1 commit 2024-06-13 16:47:01 +08:00
sb10q reviewed 2024-06-13 18:15:26 +08:00
nac3artiq/src/symbol_resolver.rs
@ -603,3 +613,1 @@
if object_id == def_id
&& constructor.is_some()
&& methods.iter().any(|(s, _, _)| s == &"__init__".into())
if let Some(rear_guard) = def.try_read() {
sb10q commented 2024-06-13 18:15:26 +08:00
Owner
Copy Link

rear_guard ?

rear_guard ?
abdul124 commented 2024-06-13 18:58:11 +08:00
Author
Collaborator
Copy Link

The previous code entered deadlock since the last element inside the def was locked (not sure about the reason for this), so to avoid that, I replaced the read() function with try_read to prevent the blocking behavior.

The previous code entered deadlock since the last element inside the def was locked (not sure about the reason for this), so to avoid that, I replaced the _read()_ function with _try_read_ to prevent the blocking behavior.
sb10q commented 2024-06-13 18:59:10 +08:00
Owner
Copy Link

Sounds like the code would then fail randomly instead of deadlocking, then?

Sounds like the code would then fail randomly instead of deadlocking, then?
abdul124 commented 2024-06-13 19:58:18 +08:00
Author
Collaborator
Copy Link

From what I have tested, only the last element inside the class definitions was locked. I was unable to see what it corresponded to in the class, but methods including the constructor were already processed. I think avoiding deadlock here should be more important as the code after it can give more meaningful error messages. I will further look into the reason for this blocking and update here.

From what I have tested, only the last element inside the class definitions was locked. I was unable to see what it corresponded to in the class, but methods including the constructor were already processed. I think avoiding deadlock here should be more important as the code after it can give more meaningful error messages. I will further look into the reason for this blocking and update here.
sb10q commented 2024-06-13 20:38:54 +08:00
Owner
Copy Link

Please break it into another PR while you investigate it.

Please break it into another PR while you investigate it.
sb10q reviewed 2024-06-13 18:23:47 +08:00
nac3artiq/src/symbol_resolver.rs
@ -302,2 +304,3 @@
self.helper.id_fn.call1(py, (self.helper.type_fn.call1(py, (pyty,))?,))?.extract(py)?;
let py_obj_id: u64 = self.helper.id_fn.call1(py, (pyty,))?.extract(py)?;
let get_def_id = || {
sb10q commented 2024-06-13 18:23:47 +08:00
Owner
Copy Link

This is only used once. Worth defining?

This is only used once. Worth defining?
abdul124 commented 2024-06-13 19:03:32 +08:00
Author
Collaborator
Copy Link

Added this at the top for clarity in if condition, but yes since it is used only once, will rewrite this and move it with the calling condition.

Added this at the top for clarity in if condition, but yes since it is used only once, will rewrite this and move it with the calling condition.
sb10q commented 2024-06-13 18:30:15 +08:00
Owner
Copy Link

The PR text should be the commit log. And shouldn't "Updated Constructor function detection in get_obj_type function to avoid deadlock when performing read on definitions" be split into another PR?

The PR text should be the commit log. And shouldn't "Updated Constructor function detection in get_obj_type function to avoid deadlock when performing read on definitions" be split into another PR?
sb10q requested review from derppening 2024-06-13 18:30:33 +08:00
abdul124 changed title from support_string_class_attr to WIP: support_string_class_attr 2024-06-13 21:45:23 +08:00
abdul124 commented 2024-06-14 16:02:03 +08:00
Author
Collaborator
Copy Link

Splitting PR for Clarity
I have split the PR into two separate PRs for better clarity and management.

  1. PR #416: This PR focuses on supporting string attributes
  2. PR #417: This PR focuses on allowing class objects for classes without proper __init__ function (#102)

I am closing the current PR, #415, as its contents are now represented by the two new PRs listed above.

**Splitting PR for Clarity** I have split the PR into two separate PRs for better clarity and management. 1. PR #416: This PR focuses on supporting string attributes 2. PR #417: This PR focuses on allowing class objects for classes without proper _\_\_init\_\__ function (#102) I am closing the current PR, #415, as its contents are now represented by the two new PRs listed above.
abdul124 closed this pull request 2024-06-14 16:02:03 +08:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
derppening
Labels
Clear labels   
UROP0622

   
future-extension

   
high-priority

   
low-priority
No Label
UROP0622
future-extension
high-priority
low-priority
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/nac3#415
No description provided.
Delete Branch "support_string_class_attr"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?