Skip to content

Conversation

tsnobip
Copy link
Member

@tsnobip tsnobip commented Jan 6, 2024

Fixes #6549.

As suggested by @anmonteiro in his comment of issue #6438, I started with adding the remaining part from melange-re/melange#732 , but this is not enough.

Edit: it works now :)

@tsnobip tsnobip marked this pull request as draft January 6, 2024 18:33
}

function getName$p(t) {
return t.name;
Copy link
Member Author

Choose a reason for hiding this comment

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

you can see the error here, should be:

Suggested change
return t.name;
return t.renamed;

@anmonteiro
Copy link
Contributor

Just adding the functions doesn't work, you need to actually call them https://github.com/melange-re/melange-compiler-libs/pull/26/files

@tsnobip
Copy link
Member Author

tsnobip commented Jan 15, 2024

Just adding the functions doesn't work, you need to actually call them https://github.com/melange-re/melange-compiler-libs/pull/26/files

you're the best @anmonteiro, thanks a lot, it's working now!

@tsnobip tsnobip changed the title [WIP] Try to fix renamed field access in inline records Fix renamed field access in inline records Jan 15, 2024
@tsnobip tsnobip marked this pull request as ready for review January 15, 2024 09:36
@tsnobip tsnobip requested a review from cristianoc January 15, 2024 09:38
@tsnobip tsnobip force-pushed the renamed_inline_record_field_access branch from 9419c6d to 6a2c168 Compare January 15, 2024 10:05
@tsnobip tsnobip changed the base branch from master to 11.0_release January 15, 2024 10:05

Bug fixes and maintenance should target branch `11.0_release`.
We'll merge `11.0_release` into `master` from time to time to propagate those changes.

Copy link
Member

Choose a reason for hiding this comment

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

👍

@tsnobip tsnobip force-pushed the renamed_inline_record_field_access branch from 328b3c9 to afd2b59 Compare January 15, 2024 12:13
@tsnobip tsnobip enabled auto-merge (squash) January 15, 2024 12:14
Lambda.Blk_record
{ fields = all_labels_info; mutable_flag = mut; record_repr }

let blk_record_ext fields mutable_flag =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this function is assigned to a reference but never used elsewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

it is used in translcore in translcore on line 1240

Record_attributes_check.check_duplicated_labels;
Lambda.fld_record := Record_attributes_check.fld_record;
Lambda.fld_record_set := Record_attributes_check.fld_record_set;
Lambda.fld_record_inline := Record_attributes_check.fld_record_inline;
Copy link
Collaborator

Choose a reason for hiding this comment

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

All this indirection with references seems unnecessary.
It was probably used a long time ago to keep the standard version of the compiler, but currently there's only 1 version of the compiler used.

Copy link
Member Author

Choose a reason for hiding this comment

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

So I should just use direct bindings? OK I'll adapt the PR

@tsnobip tsnobip force-pushed the renamed_inline_record_field_access branch from 21eebc3 to 6112b5b Compare January 17, 2024 21:00
@tsnobip tsnobip requested a review from cristianoc January 18, 2024 10:57
Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

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

Looks great.
If functions are uses in a single module only, I would even consider moving them to that module, instead of Lambda.

@tsnobip tsnobip merged commit d69d438 into 11.0_release Jan 18, 2024
@tsnobip tsnobip deleted the renamed_inline_record_field_access branch January 18, 2024 14:00
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.

Anonymous records from variant not respecting @as decorator

4 participants