M-Labs/nac3
M-Labs
/
nac3
8
0
Fork 10
Code Issues 99 Pull Requests 20 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 +15 -10
1 changed files with 15 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 459be94526 - Show all commits

15
nac3artiq/src/symbol_resolver.rs
View File

@ -304,7 +304,10 @@ impl InnerResolver {
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
Review
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
Review
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.
self.pyid_to_def.read().get(&ty_id).copied()
self.pyid_to_def
.read()
.get(&ty_id)
.copied()
.or_else(|| self.pyid_to_def.read().get(&py_obj_id).copied())
};
if ty_id == self.primitive_ids.int || ty_id == self.primitive_ids.int32 {
@ -608,10 +611,12 @@ impl InnerResolver {
let constructor_ty = pyid_to_def.get(&py_obj_id).and_then(|def_id| {
defs.iter().find_map(|def| {
if let Some(rear_guard) = def.try_read() {
sb10q commented 2024-06-13 18:15:26 +08:00
Review
Copy Link

rear_guard ?

rear_guard ?
abdul124 commented 2024-06-13 18:58:11 +08:00
Review
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
Review
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
Review
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
Review
Copy Link

Please break it into another PR while you investigate it.

Please break it into another PR while you investigate it.
if let TopLevelDef::Class {
object_id, methods, constructor, ..
} = &*rear_guard {
if object_id == def_id && constructor.is_some() && methods.iter().any(|(s, _, _)| s == &"__init__".into()) {
if let TopLevelDef::Class { object_id, methods, constructor, .. } = &*rear_guard
{
if object_id == def_id
&& constructor.is_some()
&& methods.iter().any(|(s, _, _)| s == &"__init__".into())
{
return *constructor;
}
}