-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Partial fix for invalid syntax in emitted precompile statements #28808 #32610
base: master
Are you sure you want to change the base?
Conversation
@JeffBezanson, could you look at this? |
n += ninc; | ||
jl_uv_puts(out, buf, ninc); | ||
} | ||
n += sz; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the n += ninc
inside the loop sufficient to count the amount of output?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also would be good to use this code for showing strings as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like a good thing to put in a helper function
n += jl_static_show_x(out, (jl_value_t*)vt, depth); | ||
n += jl_printf(out, ", 0x"); | ||
uint32_t a = 0x01020304; | ||
uint8_t *endian_bom = (uint8_t*) &a; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is undefined behavior (since it is observably different on different platforms). use the standard macro (BYTE_ORDER
)
n += jl_printf(out, "%02" PRIx8, data[i]); | ||
} | ||
else { | ||
n += jl_printf(out, "reinterpret("); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
n += jl_printf(out, "reinterpret("); | |
n += jl_printf(out, "bitcast("); |
uint32_t a = 0x01020304; | ||
uint8_t *endian_bom = (uint8_t*) &a; | ||
if (*endian_bom == 0x01) { // big endian | ||
for(int i = 0; i < nb; ++i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for(int i = 0; i < nb; ++i) | |
for (int i = 0; i < nb; ++i) |
for(int i = nb - 1; i >= 0; --i) | ||
n += jl_printf(out, "%02" PRIx8, data[i]); | ||
} else { | ||
assert(0); // unsupported endianness |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use a compile-time error
n += ninc; | ||
jl_uv_puts(out, buf, ninc); | ||
} | ||
n += sz; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like a good thing to put in a helper function
Bump; it'd be great to get this fixed |
Sorry for not getting back to this yet. I'll adopt the requested changes as soon as I finished my thesis in 2 weeks. |
Bump again :) |
This pull request fixes some of the issues encoutered in #28808, namely incorrect output from
--trace-compile
. Just runjulia --trace-compile=stderr
with the following example codes to see the altered behaviour.Case 1:
Previous output:
New output:
Case 2:
Previous output:
New output:
I used reinterpret for primitive types instead of directly calling the constructor here (which to my understanding is wrong since the constructor can do whatever it likes with the argument). This should work correctly even for user defined primitive types.