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

inference: follow up #44515, correctly encode Effects.overlayed effect #44634

Merged
merged 1 commit into from
Mar 17, 2022

Conversation

aviatesk
Copy link
Member

@aviatesk aviatesk requested a review from Keno March 16, 2022 07:23
@aviatesk aviatesk added the backport 1.8 Change should be backported to release-1.8 label Mar 16, 2022
@aviatesk aviatesk merged commit 1e64682 into master Mar 17, 2022
@aviatesk aviatesk deleted the avi/follow44515 branch March 17, 2022 01:07
@aviatesk aviatesk mentioned this pull request Mar 17, 2022
14 tasks
@aviatesk aviatesk removed the backport 1.8 Change should be backported to release-1.8 label Mar 17, 2022
Comment on lines +112 to +123
static void write_uint32(ios_t *s, uint32_t i) JL_NOTSAFEPOINT
{
ios_write(s, (char*)&i, 4);
}

static uint32_t read_uint32(ios_t *s) JL_NOTSAFEPOINT
{
uint32_t x = 0;
ios_read(s, (char*)&x, 4);
return x;
}

Copy link
Contributor

@giordano giordano Mar 24, 2022

Choose a reason for hiding this comment

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

These functions are duplicates of

julia/src/staticdata.c

Lines 371 to 381 in c503611

static void write_uint32(ios_t *s, uint32_t i) JL_NOTSAFEPOINT
{
ios_write(s, (char*)&i, 4);
}
static uint32_t read_uint32(ios_t *s) JL_NOTSAFEPOINT
{
uint32_t x = 0;
ios_read(s, (char*)&x, 4);
return x;
}
Also, these function introduced a new warning when compiling julia: this header file is included by ircode.c which doesn't use these functions. I'm not even sure why there are functions implementations in a header.

Copy link
Member Author

Choose a reason for hiding this comment

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

What is the best way in C to define utility functions in a single place and import them from several places? In this case, dump.c uses all write_xxx utilities defined in serialize.h while ircode.c only uses some of them and so #include serialize.h produced that warning (and the same thing would happen for staticdata.c if we remove the duplications).

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the best way in C to define utility functions in a single place and import them from several places?

Move the implementations (not the prototypes!) to a .c file, and make the files which need those functions depend on its corresponding object file in src/Makefile

Copy link
Member

Choose a reason for hiding this comment

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

Isn't the reason it is done like this to make the function definition available in all translation units and thus be eligible for inlining? But these functions are not marked inline though... But, to me, for consistency, the function in staticdata.c should be removed and this header included there instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

4 participants