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

Fixing dynamic fields #71

Closed
wants to merge 5 commits into from

Conversation

jcivlin
Copy link
Collaborator

@jcivlin jcivlin commented Jun 19, 2024

Added test dynamic-struct-example.move explains the problem:

  • if a generic structure was instantiated explicitly it compiles Ok.
  • If it is instantiated in a call to a generic function then its' instantiation is processed in our compiler only at the time when we process the function body, this structure has not been processed before, it's signature type is unknown and compiler fails.

The solution is to instantiate this structure right in the instantiation of the generic function.

Note. The key change in this PR is in added else case in module_context.rs:734.

See more in #72

@jcivlin jcivlin requested a review from ksolana June 19, 2024 01:27
let dump = ll_sty.dump_to_string();
debug!(target: "debug", "Dumping translated structure {ll_name} {}", dump);
assert!(!self.llvm_cx.named_struct_type(&ll_name).is_none(), "At this point struct {} should already get some type", &ll_name);
ll_sty
Copy link
Collaborator

Choose a reason for hiding this comment

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

Neat!

@@ -128,7 +128,8 @@ impl<'up> GlobalContext<'up> {
#[cfg(feature = "solana")]
assert!(account_address::AccountAddress::ZERO.len() == 32);

debug!(target: "globalenv", "{:#?}", env);
// Note: printing global env genarates huge output, but you can do it with the following line
// debug!(target: "globalenv", "{:#?}", env);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should just remove this line. Anyone can add debug if they want to inspect.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This comment points to the exact place where printing this info is the most useful (but really bulky).
I do not want to investigate next time I debug compiler where would be the best place for this line. So that is why I left this comment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's why we have a way to specify target: keys no? we can leave this uncommented with a specific target key. https://docs.rs/tracing/0.1.37/tracing/index.html#for-log-users

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you look at your next comment you'll see that this debug! is operated by "nodes", but it will be printed for target "debug" as well. And I need target debug while debugging...
But any way, I do not fight for this minor issue, this PR does not worth it. I'll just keep this code unchanged.

.get_nodes()
.iter()
.map(|nd| (*nd, g_env.get_node_type(*nd)))
.collect();
debug!(target: "nodes", "\n{:#?}", &map_node_to_type);
// NOTE. Printing nodes is reserved for the future development of compiler
// debug!(target: "nodes", "\n{:#?}", &_map_node_to_type);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here. Let's delete this line.

@@ -665,6 +673,13 @@ impl<'mm, 'up> FunctionContext<'mm, 'up> {
.into_struct(*struct_id);
assert_eq!(dst.len(), 1);
assert_eq!(src.len(), senv.get_field_count());
let s_full_name = senv.get_full_name_str();
Copy link
Collaborator

@ksolana ksolana Jun 19, 2024

Choose a reason for hiding this comment

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

We should outline this code in a separate function as this is solely for debugging.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did not understand this comment. Please explain.

Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry for the typo. lines 676-682 seems to be used only for debugging purposes. we can outline this to a separate function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, adding a printing fields function for StructEnv.

@jcivlin
Copy link
Collaborator Author

jcivlin commented Jun 21, 2024

This PR was superseded by #73. Closing it without merge.

@jcivlin jcivlin closed this Jun 21, 2024
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