Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add new intrinsic
is_constant
and optimizepow
#114390Add new intrinsic
is_constant
and optimizepow
#114390Changes from all commits
abeda62
4aa0608
2605ea7
f569cea
1b75669
79016f8
4f31013
cf9406f
be35381
c1adc30
87b5cea
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Shouldn't the
black_box
be the inner call?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.
Either way, the behavior is unspecified, even without
black_box
. imo I think it being outside the call makes more sense since it conveys better that optimizations may change the result, while callingis_val_statically_known
onblack_box
may returnfalse
because it's a function call, or smth.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.
What about entirely removing black_box from the example?
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.
Sounds good
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.
Both doc tests fail when ran with
./x test core --stage 1 --doc -- is_val_statically_known
. The doc tests are never ran with--stage 0
because of#[cfg(not(bootstrap())]
.You need to add
unsafe
,use
, andfeature
statements to make them compile. If you think they should be omitted from the docs, then you can prepend them with#
.Here is an example that passes the tests:
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.
Are they allowed to differ in panics? E.g. if an invalid input is handed to a safe function then it may either panic or return garbage and this differs between the branches?
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.
Adding onto this, are panic messages/locations allowed to differ (if it always panics with the same inputs in CTFE/runtime)? They do in
pow
.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.
I'd say they must either both panic or neither panic. But the exact panic location is not relevant.
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.
That should be documented then. Though I think it might be tricky to uphold. Would it be possible to have
is_val_statically_known
to always stick to one value in const eval? That would be less footgun-ish.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.
we could (in debug mode?) fork the const evaluator and run both paths and compare the result 🙃
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.
It's the same with
const_eval_select
. We already have an RFC up for resolving this properly; in the mean time I think we should keep the strict rules.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.
Could
#[track_caller]
and custom panic messages fix this? For that matter, is there a reasonpow
didn't have#[track_caller]
to start out with?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.
Don't think there's any reason not to. Custom panic messages would be a nice addition also
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.
I want to add, for the benefit of those reading in the future, that the core library already intentionally gives different panic messages depending on
const_eval_select
. It appears this is for performance reasons because the runtime variant is static, while the compile time variant is not. As far as I know, that is the only current use ofconst_eval_select
.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.
It's used for a couple other things as well, like having certain assertions only in the run-time version of some low-level functions (
assert_unsafe_precondition
), and const-ifying functions whose runtime panic message needs the formatting machinery which is not supported at compile-time.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.
What is
forget
okay here? Does the real intrinsic also forget its argument? Should we restrict it to Copy types to avoid the confusion that could cause?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.
Copy
is fine. This iscfg(bootstrap)
so it should be gone by the next beta release anyway.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.
yea that may be better actually ^^ We don't support anything else in the backend anyway
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.
Should be
Copy
on both of them then.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.
is incorrect to use
#[cfg(debug_assertions)]
here. In general, we can't assumedebug_assertions
matchesoverflow_checks
even though they go together by default. This is especially true because this function has the#[rustc_inherit_overflow_checks]
.Without
#[cfg(debug_assertions)]
, it is still incomplete because the greatest possible u32 is much greater than the number of bits in an integer.To correctly check for overflow, test to see if the final result is less than 1. If that is the case use
_ = <Self>::MAX * <Self>::MAX;
to force a panic iffoverflow_checks
are enabled.Although it isn't necessary, you can make the code less verbose by using
saturating_mul
instead ofoverflowing_mul
and by using(1 as Self).checked_shl().unwrap_or_default()
instead of(1 << num_shl) * fine as Self
.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.
just a nit to make it visually more clear that these are all together testing one thing
EDIT: hm I tried to remove the empty line, not sure if github supports that...