Fix warnings about the native target-cpu
#53642
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors r+ |
|
📌 Commit 744c6c822d9b936631f796c0d454d5d1750ce616 has been approved by |
bors
added
S-waiting-on-bors
|
Wait, this doesn't have a test test please @bors r- |
bors
added
S-waiting-on-author
arielb1
reviewed
Aug 23, 2018
| unsafe { | ||
| let mut len = 0; | ||
| let ptr = llvm::LLVMRustGetHostCPUName(&mut len); | ||
| str::from_utf8_unchecked(slice::from_raw_parts(ptr as *const u8, len)) |
There was a problem hiding this comment.
I'm also a bit annoyed that you are using from_utf8_unchecked here instead of from_utf8.unwrap
There was a problem hiding this comment.
not that I think this failing is unlikely, except for API bugs, but minus 5 points for every use of unchecked.
alexcrichton
force-pushed
the
fix-target-cpu-native
branch
from
744c6c8
to
69b6ad1
Compare
|
@bors: r=arielb1 |
|
📌 Commit 69b6ad1ee44f66dfe621daec5ef076793ce23406 has been approved by |
bors
added
S-waiting-on-bors
|
@bors r- |
bors
added
S-waiting-on-author
arielb1
reviewed
Aug 24, 2018
| $(call RUN,foo) | ||
| # Make sure no warnings about "unknown CPU `native`" were emitted | ||
| if grep 'is not a recognized processor' $(TMPDIR)/out.log; then exit 1; fi |
There was a problem hiding this comment.
I would prefer to check that no warnings at all are emitted (i.e. that out.log is empty), to avoid this recurring with a different error message.
I don't think the compiler should ever emit garbage when compiling an empty file.
There was a problem hiding this comment.
Are there platforms where the compiler is always chatty? If then, I would check that the compiler is not chatty on all the other platforms.
There was a problem hiding this comment.
I basically hate writing tests in bash, do you know how to test a file is empty?
There was a problem hiding this comment.
Perhaps use wc and count the number of characters?
wc --chars file | cut -d' ' -f1 should be 0.
There was a problem hiding this comment.
How do you test against 0 though? (I'm really bad at bash...)
There was a problem hiding this comment.
#!/bin/bash
touch file
if [ $(wc --chars file | cut -d' ' -f1) == 0 ]; then
echo "it's zero!"
else
echo "it's not zero!"
fishould work :)
There was a problem hiding this comment.
Wouldn't -s FILE work? "True if FILE exists and has a size greater than zero."
|
r=me with test changed. |
alexcrichton
force-pushed
the
fix-target-cpu-native
branch
from
69b6ad1
to
715d2cc
Compare
|
@bors: r=arielb1 |
|
📌 Commit 715d2cc has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Aug 27, 2018
Fix warnings about the `native` target-cpu This fixes a regression from #53031 where specifying `-C target-cpu=native` is printing a lot of warnings from LLVM about `native` being an unknown CPU. It turns out that `native` is indeed an unknown CPU and we have to perform a mapping to an actual CPU name, but this mapping is only performed in one location rather than all locations we inform LLVM about the target CPU. This commit centralizes the mapping of `native` to LLVM's value of the native CPU, ensuring that all locations we inform LLVM about the `target-cpu` it's never `native`. Closes #53322
|
💔 Test failed - status-travis |
bors
added a commit
that referenced
this pull request
Aug 28, 2018
Fix warnings about the `native` target-cpu This fixes a regression from #53031 where specifying `-C target-cpu=native` is printing a lot of warnings from LLVM about `native` being an unknown CPU. It turns out that `native` is indeed an unknown CPU and we have to perform a mapping to an actual CPU name, but this mapping is only performed in one location rather than all locations we inform LLVM about the target CPU. This commit centralizes the mapping of `native` to LLVM's value of the native CPU, ensuring that all locations we inform LLVM about the `target-cpu` it's never `native`. Closes #53322
|
💔 Test failed - status-travis |
bors
added
S-waiting-on-review
|
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 |
kennytm
added
S-waiting-on-author
This fixes a regression from rust-lang#53031 where specifying `-C target-cpu=native` is printing a lot of warnings from LLVM about `native` being an unknown CPU. It turns out that `native` is indeed an unknown CPU and we have to perform a mapping to an actual CPU name, but this mapping is only performed in one location rather than all locations we inform LLVM about the target CPU. This commit centralizes the mapping of `native` to LLVM's value of the native CPU, ensuring that all locations we inform LLVM about the `target-cpu` it's never `native`. Closes rust-lang#53322
alexcrichton
force-pushed
the
fix-target-cpu-native
branch
from
12e33bf
to
1fd45a1
Compare
|
@bors: r=arielb1 |
|
📌 Commit 1fd45a1 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 1fd45a1 with merge fe357694fbbc9bab93870e2c3a3f28195d1ff75d... |
|
💔 Test failed - status-travis |
bors
added
S-waiting-on-review
|
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 |
|
@bors: retry |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Aug 29, 2018
Fix warnings about the `native` target-cpu This fixes a regression from #53031 where specifying `-C target-cpu=native` is printing a lot of warnings from LLVM about `native` being an unknown CPU. It turns out that `native` is indeed an unknown CPU and we have to perform a mapping to an actual CPU name, but this mapping is only performed in one location rather than all locations we inform LLVM about the target CPU. This commit centralizes the mapping of `native` to LLVM's value of the native CPU, ensuring that all locations we inform LLVM about the `target-cpu` it's never `native`. Closes #53322
|
☀️ Test successful - status-appveyor, status-travis |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This fixes a regression from #53031 where specifying
-C target-cpu=nativeisprinting a lot of warnings from LLVM about
nativebeing an unknown CPU. Itturns out that
nativeis indeed an unknown CPU and we have to perform amapping to an actual CPU name, but this mapping is only performed in one
location rather than all locations we inform LLVM about the target CPU.
This commit centralizes the mapping of
nativeto LLVM's value of the nativeCPU, ensuring that all locations we inform LLVM about the
target-cpuit'snever
native.Closes #53322