Uh oh!
There was an error while loading. Please reload this page.
hir: Disallow target_feature on constants - #64809
Conversation
rust-highfive
commented
Sep 26, 2019
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
Uh oh!
There was an error while loading. Please reload this page.
gnzlbg
commented
Sep 26, 2019
via email
This LGTM, target feature should only be allowed on functions and functions
only - I think we should disallow it for const fns. |
feb6cd8 to
b310f61Compare
varkor
left a comment
There was a problem hiding this comment.
Thanks! r=me after a test case for multiple invalid attributes.
Uh oh!
There was an error while loading. Please reload this page.
rust-highfive
commented
Sep 26, 2019
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
b310f61 to
a31c8bcCompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
a31c8bc to
351f551CompareUh oh!
There was an error while loading. Please reload this page.
varkor
commented
Sep 26, 2019
r=me when tests pass (with or without the final suggestions, which are entirely preferential). |
This commit fixes an ICE when `target_feature` is applied to constants. Signed-off-by: David Wood <david@davidtw.co>
See linked comment[1] for context. 1: rust-lang#64809 (comment) Signed-off-by: David Wood <david@davidtw.co>
351f551 to
c3368bdComparegnzlbg
commented
Sep 26, 2019
via email
Of note: The reason I was attempting to use target function on a const was because the const needed a different value on different targets.
For AVX2 targets, it needed a value of 7, for AVX512 targets, a value of
15, and for other targets, a value of 3. Pointers on how to do it right are
appreciated.
You can use #[cfg(target_feature)] for that. |
moonheart08
commented
Sep 26, 2019
I had deleted my comment and moved it to the issue that caused this PR, thank you, however! |
estebank
commented
Sep 27, 2019
@bors r=varkor |
bors
commented
Sep 27, 2019
📌 Commit c3368bd has been approved by |
…e-const, r=varkor hir: Disallow `target_feature` on constants Fixesrust-lang#64768. This PR fixes an ICE when `#[target_feature]` is applied to constants by disallowing this with the same error as when `#[target_feature]` is applied to other places it shouldn't be. I couldn't see anything in the [RFC](https://github.com/rust-lang/rfcs/blob/master/text/2045-target-feature.md) that suggested that `#[target_feature]` should be applicable to constants or any tests that suggested it should, though I might have missed something - if this is desirable in future, it remains possible to remove this error (but for the time being, I think this error is better than an ICE). I also added some extra cases to the test for other places where `#[target_feature]` should not be permitted. cc @gnzlbg
Rollup of 14 pull requests Successful merges: - #63492 (Remove redundancy from the implementation of C variadics.) - #64703 (Docs: slice elements are equidistant) - #64745 (Include message on tests that should panic but do not) - #64781 (Remove stray references to the old global tcx) - #64794 (Remove unused DepTrackingMap) - #64802 (Account for tail expressions when pointing at return type) - #64809 (hir: Disallow `target_feature` on constants) - #64815 (Fix div_duration() marked as stable by mistake) - #64818 (update rtpSpawn's parameters type(It's prototype has been updated in libc)) - #64830 (Thou shallt not `.abort_if_errors()`) - #64836 (Stabilize map_get_key_value feature) - #64845 (pin.rs: fix links to primitives in documentation) - #64847 (Upgrade env_logger to 0.7) - #64851 (Add mailmap entry for Dustin Bensing by request) Failed merges: - #64824 (No StableHasherResult everywhere) r? @ghost
…e-const, r=varkor hir: Disallow `target_feature` on constants Fixesrust-lang#64768. This PR fixes an ICE when `#[target_feature]` is applied to constants by disallowing this with the same error as when `#[target_feature]` is applied to other places it shouldn't be. I couldn't see anything in the [RFC](https://github.com/rust-lang/rfcs/blob/master/text/2045-target-feature.md) that suggested that `#[target_feature]` should be applicable to constants or any tests that suggested it should, though I might have missed something - if this is desirable in future, it remains possible to remove this error (but for the time being, I think this error is better than an ICE). I also added some extra cases to the test for other places where `#[target_feature]` should not be permitted. cc @gnzlbg
Rollup of 14 pull requests Successful merges: - #64703 (Docs: slice elements are equidistant) - #64745 (Include message on tests that should panic but do not) - #64781 (Remove stray references to the old global tcx) - #64794 (Remove unused DepTrackingMap) - #64802 (Account for tail expressions when pointing at return type) - #64809 (hir: Disallow `target_feature` on constants) - #64815 (Fix div_duration() marked as stable by mistake) - #64818 (update rtpSpawn's parameters type(It's prototype has been updated in libc)) - #64830 (Thou shallt not `.abort_if_errors()`) - #64836 (Stabilize map_get_key_value feature) - #64845 (pin.rs: fix links to primitives in documentation) - #64847 (Upgrade env_logger to 0.7) - #64851 (Add mailmap entry for Dustin Bensing by request) - #64859 (check_match: improve diagnostics for `let A = 2;` with `const A: i32 = 3`) Failed merges: r? @ghost
See linked comment[1] for context. 1: rust-lang#64809 (comment) Signed-off-by: David Wood <david@davidtw.co>
Fixes#64768.
This PR fixes an ICE when
#[target_feature]is applied to constants by disallowing this with the same error as when#[target_feature]is applied to other places it shouldn't be.I couldn't see anything in the RFC that suggested that
#[target_feature]should be applicable to constants or any tests that suggested it should, though I might have missed something - if this is desirable in future, it remains possible to remove this error (but for the time being, I think this error is better than an ICE).I also added some extra cases to the test for other places where
#[target_feature]should not be permitted.cc @gnzlbg